I don't have experience in writing tests in jest but looks reasonable

Actually remove the remaining "only for flow" lines, not sure why this didn't commit before...

Task here: ENG-3377

This diff seems to call getAPNsNotificationTopic with two parameters, but the version of that function in master only takes one parameter. Can you please update this diff's dependencies so I can see the rest of your stack? Please always set diff dependencies!

Remove the remaining // only for flow lines

I've left some inline comments about the types themselves. More general notes:

Remove unnecessary lines

This code was supposed to handle an edge case where:

1. Electron downloads an update, and we display the modal
2. Before the user presses the modal to quit-and-install, we release a new version.

Without this, the user will have to update the app twice instead of once (in this rare case).

Rebase

Looks good to me

I'm not sure if it is the case. For now, displaying both 'delivery failed' and the label ('Edited') at the same time is impossible.

Oh yeah, that makes sense, sorry!

Change order

Good point, I've added a check that makes sure that the timer doesn't already exist before creating a new one

Rebase

Can you check what it looks like with the "Delivery failed. RETRY?" text which appears when sending a message, and attach a screenshot? Not sure if this case was considered in the designs.

Looks good to me, but I haven't worked with this code so it would be nice if someone else could also take a look.

Abandoned in favour of D7032

Abandoned in favour of D7032

Fix nit

prepareWebNotification now takes a bag of parameters

Rerun CI

Extract platformDetails variable

Fix typo.
The types aren't readonly because either:

• they are class methods and can't be made readonly
• the NotificationEvents type isn't actually ever instantiated or exported, it's just a holder that maps event string name to the callback type

Add comment, tasks: ENG-3173, ENG-3174

Extract platformDetails variables.

Type the json from the web push API. The PushMessageData is taken from the official web api types so I think it's better to keep them in sync and only later type them as our own type.

Move WebNotification type to lib so we can use it to type the push message in the service worker.

Update import

Change to export default

Fix types

@inka was right, I forgot a return for the cleanup callback. Also removed one ? but the second one is still needed because there's a function call after condition so flow can't be sure if the variable wasn't modified.

Add newline and react memo.

Moved eslint annotation to a later diff.

Fix types, move errors codes, answers to questions in inline comments.

Moved the code for the device token reducer to lib in a previous diff. Moved the device token to BaseAppState and reduce it in the baseReducer.

Added webpack.DefinePlugin with some constants (NODE_ENV, BROWSER). Moved part of the configuration to the shared.cjs so it's together with similar comfigurations for web webpack.

Rebase, move the web-push dependency from future diff to this one, and init it during config loading.

If possible, I think it would be best if only the tab that is being brought to focus navigates to the notification. The rest of them should stay where they are

Yeah, that's how it's going to work

Rebase

Rebase

Don't print in development, print warning instead of invariant in production (the previous code was inspired by our apn handling of secrets)

There's no global exports set and "the bundle is just going to be run directly by the browser" is correct, so I renamed the file to .js.
Additionaly I'm bringing back the Service-Worker-Allowed header because (now that I understand it better) this service worker should have all tabs in scope and needs to be able to claim them all (for example to focus on them after clicking the notification). It's not like it handles only some part of the website.

• Changed suffix from .cjs to .js
• Added babel config to strip flow types

Moved the code to the push/providers.js and use importJSON. I'm no longer caching the config myself because importJSON does the caching. Added logging and invariant.

I'm a bit confused here. commonjs2 isn't an option for target [...]

Sorry, I shouldn't have used the word "target", I meant this option: output.module. In the generated file I can see some webpack boilerplate that defines properties on the exports object so the file should be cjs.

• Changed formatting, remove Service-Worker-Allowed header (it's actually not needed if we don't specify a custom scope during registration)

@ashoat:

• Tested on prod mode and amended the test plan
• Hot-reloading was already discussed in D6715
• From what I understand the default target for webpack is CommonJS so .cjs should be correct.