Added types to the native notifs module based on the typescript types generated by NodeRT and on the Windows API documentation.
Details
Details
- Reviewers
kamil tomek - Commits
- rCOMM750e68fa633a: [desktop] Type windows notifs module
Run flow in desktop
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| desktop/flow-typed/@commapp/windowspush_vx.x.x.js | ||
|---|---|---|
| 82 ↗ | (On Diff #24721) | This is typed as an Array<number> in typescript and the underlying Windows API is byte[] but it doesn't behave like an array during runtime. I've tried Array.isArray and checking if it was a TypedArray but couldn't get a definite answer. It isn't an iterator, but it has length and can be indexed with numbers. |
| desktop/flow-typed/@commapp/windowspush_vx.x.x.js | ||
|---|---|---|
| 82 ↗ | (On Diff #24721) | Perhaps it should be typed this way? +payload: { +[number]: number, +length: number, }, |
Comment Actions
Note that an alternative to a flow-typed library would be to type the library directly. If the code instead the library has // @flow annotations, the libdef would not be necessary. Not sure if that's easier or harder though... could be harder to do if the library is largely codegenned code
Comment Actions
Some questions about types, and please update the declaration of payload.
| desktop/flow-typed/@commapp/windowspush_vx.x.x.js | ||
|---|---|---|
| 51–78 ↗ | (On Diff #24721) | What is the meaning of these methods? Add and remove listener are quite obvious, but then on sounds like something which should work just as addListener... and off is maybe removeListener? What is the purpose of on and off? |
| 86–95 ↗ | (On Diff #24721) | It is surprising to see these methods, as this type is an argument of listener methods from PushNotificationManager. So it seems like that listeners will be called with something that can be listened on, which is strange. |
| desktop/flow-typed/@commapp/windowspush_vx.x.x.js | ||
|---|---|---|
| 51–78 ↗ | (On Diff #24721) | There are just standard aliases for node's EventEmitter |
| 86–95 ↗ | (On Diff #24721) | This is used when handling notifications when the app isn't running (docs). This doesn't apply to our app, but I've left them because they were generated by NodeRT. |