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

fix(afs): fix di warning #1401

Merged
merged 1 commit into from
Jan 3, 2018
Merged

fix(afs): fix di warning #1401

merged 1 commit into from
Jan 3, 2018

Conversation

FrozenPandaz
Copy link
Contributor

Checklist

Description

Fixing Error with Dependency Injection not being able to resolve all parameters in an Injectable class.

AngularFirestore is an Injectable class meaning its parameters must be resolvable via Dependency Injection.

Its current inability to do this, results in the warning:

Warning: Can't resolve all parameters for AngularFirestore in /XXXProjectPathXXX/node_modules/angularfire2/firestore/index.d.ts: ([object Object], ?). This will become an error in Angular v6.x

Using the injected Token allows the shouldEnablePersistence parameter to be resolvable via DI thus fixing the above warning!

Implementation Caveat:

  • EnablePersistenceToken had to be moved to its own file in order to prevent circular module dependencies. A possible alternative would be to export this token from firestore.ts

Bonus:

  • Slightly refactored the definition of FirestoreModule to only provide AngularFirestore as a provider
    • The EnablePersistenceToken only gets injected if it the enablePersistence function is called

Code sample

No API Changes

@davideast
Copy link
Member

Oh my gosh, thank you so much @FrozenPandaz! This was on the top of my "stuff I need to actually do list", so I can't thank you enough.

@davideast davideast merged commit 23ab383 into angular:master Jan 3, 2018
@FrozenPandaz FrozenPandaz deleted the fix1206 branch January 3, 2018 18:26
myspivey pushed a commit to myspivey/angularfire2 that referenced this pull request Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants