User Details
- User Since
- Sep 8 2022, 2:37 AM (28 w, 3 d)
Fri, Mar 24
I don't have experience in writing tests in jest but looks reasonable
Thu, Mar 23
Actually remove the remaining "only for flow" lines, not sure why this didn't commit before...
Wed, Mar 22
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:
Mon, Mar 20
Remove unnecessary lines
This code was supposed to handle an edge case where:
- Electron downloads an update, and we display the modal
- 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).
Fri, Mar 17
Thu, Mar 16
Looks good to me
Wed, Mar 15
Tue, Mar 14
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!
Mon, Mar 13
Change order
Good point, I've added a check that makes sure that the timer doesn't already exist before creating a new one
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.
Fri, Mar 10
Abandoned in favour of D7032
Abandoned in favour of D7032
Thu, Mar 9
Wed, Mar 8
Thu, Mar 2
Wed, Mar 1
prepareWebNotification now takes a bag of parameters
Tue, Feb 28
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
Mon, Feb 27
Fri, Feb 24
Feb 24 2023
@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.
Feb 22 2023
Feb 21 2023
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
Feb 20 2023
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
Feb 17 2023
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.
Feb 15 2023
- Changed formatting, remove Service-Worker-Allowed header (it's actually not needed if we don't specify a custom scope during registration)
- 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.