Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Messaging rewrite #2484

Merged
merged 1 commit into from
Jan 9, 2020
Merged

Messaging rewrite #2484

merged 1 commit into from
Jan 9, 2020

Conversation

mmermerkaya
Copy link
Contributor

@mmermerkaya mmermerkaya commented Dec 31, 2019

I didn't want to leave the FCM SDK in it's current state, so I rewrote a large portion of it (and hopefully without any API changes this time 😄). There are also some API changes that I'd really like to do, like removing getToken and deleteToken from SWController or making vapidKey a parameter of getToken, but I guess they'll have to wait.

Changes:

  • Composition over inheritance
    • Removed BaseController that the two main controllers were extending and moved the common parts to "token manager" module.
    • Removed DbInterface, which was not actually an interface but an abstract class, and the two classes that were extending it.
  • Database
    • Moved Push API related fields to the subscriptionOptions subfield. This is now an optional field, as preparation for the eventual Safari support, which will not use the Push API.
    • Dropped swScope as an index, for the same reason as before.
    • Database key changed to App ID, as there should only be one token for every App ID. This should prevent "multiple messages received" issues in topic sends.
    • Created a new DB and added a migration function because the object store needed to change. This allowed removing the old DB code.
    • Started using the battle-tested IndexedDB library idb, which is already being used in Installations.
  • Size reduction
    • Non minified ES5 ESM build is now 70.8kB. Old one was 96.8kB.
    • Minified and gzipped ES5 build is now 10.1kB. Old one was 12.5kB.
  • Tests
    • Increased coverage from 93% to 99%. Coveralls reports a drop in coverage because I also changed the coverage config to stop including test files in coverage reports.
    • Made the tests more meaningful by removing all private member access from tests. Focusing on testing behavior, not implementation.
    • Added complete Service Worker fakes to use with every test. Removed messing with prototype objects in fakes.
    • Removed the uses of any type from tests. There used to be 83 instances of type any in tests.
  • Structure and Naming:
    • Greatly reduced the number of classes. Not everything needs to be a class.
    • Combined src/ and test/ folders, following our style guide.
    • Removed the -model suffix. More meaningful names overall.

Copy link

@dwoffinden dwoffinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! :D

config/webpack.test.js Outdated Show resolved Hide resolved
packages/messaging/src/controllers/sw-controller.ts Outdated Show resolved Hide resolved
packages/messaging/src/core/api.test.ts Show resolved Hide resolved
packages/messaging/src/controllers/window-controller.ts Outdated Show resolved Hide resolved
packages/messaging/src/controllers/window-controller.ts Outdated Show resolved Hide resolved
packages/messaging/src/interfaces/message-payload.ts Outdated Show resolved Hide resolved
packages/messaging/src/testing/compare-headers.ts Outdated Show resolved Hide resolved
packages/messaging/src/testing/compare-headers.ts Outdated Show resolved Hide resolved
packages/messaging/src/util/constants.ts Outdated Show resolved Hide resolved
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-messaging-refactor branch from d4ee9b6 to d676026 Compare January 2, 2020 17:42
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-messaging-refactor branch 2 times, most recently from 04d89c1 to 43fffee Compare January 3, 2020 16:08
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-messaging-refactor branch from 19d4497 to bffbf56 Compare January 9, 2020 09:45
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-messaging-refactor branch from bffbf56 to 0e300e9 Compare January 9, 2020 11:19
@mmermerkaya mmermerkaya merged commit f7bf249 into master Jan 9, 2020
@mmermerkaya mmermerkaya deleted the mmermerkaya-messaging-refactor branch January 9, 2020 12:03
@hsubox76 hsubox76 added this to the next milestone Jan 9, 2020
@firebase firebase locked and limited conversation to collaborators Feb 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants