Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Minor modular sample fixes #3026

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

Open
hakimio wants to merge 4 commits into angular:main
base: main
Choose a base branch
Loading
from hakimio:minor-modular-sample-fixes

Conversation

Copy link

@hakimio hakimio commented Oct 20, 2021

Description

A couple minor fixes for the modular demo:

  • "ScreenTrackingService" and "UserTrackingService" were imported but never used. Added them to app module providers.
  • Removed a couple of unused imports.

Additional info

Tried building and serving the demo project, but in both cases it throws the following error:

./node_modules/@angular/fire/fesm2015/angular-fire.js:18:104-117 - Error: export 'isSupported' (imported as 'isSupported2ドル') was not found in 'firebase/remote-config' (possible exports: activate, ensureInitialized, fetchAndActivate, fetchConfig, getAll, getBoolean, getNumber, getRemoteConfig, getString, getValue, setLogLevel)

Copy link
Member

@hakimio User/ScreenTracking are automatic, they don't need to be called to begin functioning. The isSupported error is likely cause the firebase JS SDK needs to be updated in that sample.

Copy link
Author

hakimio commented Oct 20, 2021
edited
Loading

@jamesdaniels not sure I understand what you mean by "don't need to be called". In angularfire v6, you had to add those services to the app providers. Isn't that the case in v7? How are they supposed to be used now?

EDIT: ScreenTracking service usage in v6.

Copy link
Member

@hakimio woops, don't mind me. I peek at this on mobile and it looked like you deleted those lines, rather than added them. Good catch.

Copy link
Author

hakimio commented Oct 25, 2021
edited
Loading

@jamesdaniels
Minor nitpicking:

  • instead of creating multiple subscriptions with async pipe, you could use *ngrxLet to resolve the observable or @ngneat/until-destroy to automatically unsubscribe when component is destroyed.
  • To avoid mistakes like this one, might be a good idea to rename user to user$.

Unrelated question/issue: because of includeMetadataChanges, docdata() emits duplicate values. Is there a good reason for those duplicates or can this be fixed? valueChanges() from angularfire v6 did not have this issue (I verified).

EDIT: imho, changing includeMetadataChanges from default false in Firebase JS SDK to true in rxfire and not allowing user to change it, makes no sense and just creates confusion from user side 😕

Copy link
Member

In the samples I don't think we want to take on any additional dependencies, just for the sake of being able to quickly validate correct functionality with any Angular version, including RCs, before other NPM modules are compatible.

Thanks for pointing out this is the case with docData, I mostly stick to snapshots rather than values, so I didn't notice this was different in rxfire. One thing to note, the metadata could be consumed in a class converter, so it's probably safer to keep metadata included by default... we do want this to be configurable but in the meantime I think it makes sense to build a distinctUntilChanged() into doc/collectionData.

johanchouquet reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /