-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fixing perf and fcm #2597
Fixing perf and fcm #2597
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple of nits.
These FCM changes look promising. I'll be interested to see if they solve the conflicts that have been reported with ngsw.
constructor(public readonly messaging: AngularFireMessaging, swPush: SwPush) { | ||
messaging.usePublicVapidKey('BIDPctnXHQDIjcOXxDS6qQcz-QTws7bL8v7UPgFnS1Ky5BZL3jS3-XXfxwRHmAUMOk7pXme7ttOBvVoIfX57PEo').then(() => { | ||
constructor(public readonly messaging: AngularFireMessaging, readonly swpush: SwPush) { | ||
swpush.messages.subscribe(it => console.log('swpush', it)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this just for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is in the sample app, so this looks good then! Missed that on the first pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems FCM has made some changes to the FCM API that have affected us. Experimenting with different variations on Service Workers and FCM in the sample app & will probably revamp those guides shortly.
switchMap(() => import('@firebase/performance')), | ||
tap(perf => perf.registerPerformance && perf.registerPerformance(firebase as any)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see these aren't needed. I did some testing to see if they were causing issues, but they weren't... though I'm not sure if they were doing anything useful either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah these register calls aren't great... it's weird and they're private APIs, so the JS SDK team can change them at any point.
I've found that I've needed them for some SSR cases but it seems marking everything as an external in angular.json makes it so they aren't always needed. In either case, perf mon is only browser, so I can drop this. I believe the root problem is due to ngcc failing when it hits the CJS version of Firebase. It's somehow messing with the side-effects of the import. We've tried to address a couple times but keep running into issues. That said, we have a tree-shakable version of the JS SDK on the way https://github.com/firebase/firebase-js-sdk/blob/master/docs-exp/index.md that should play much much better with Angular; so I don't think it's worth spending much time trying to solving the CJS / tree-shake problems with NG10 & the namespaced JS SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great news on the tree-shakable JS SDK 👍 Sounds like there will be plenty of refactoring later this year or next year 😁
Checklist
yarn install
,yarn test
run successfully? (yes/no; required)Description
Code sample