Explicitly say that 'firebase/firestore' must be imported

[marob]

Checklist

• Issue number for this PR: Quickstart doesn't explicitly say to import 'firebase/firestore' #2332

Description

It is easy to miss the 'firebase/firestore' import and don't understand why this quickstart doesn't work.

[Splaktar]

LGTM

[davideast]

Thanks for sending in a PR, but this should not be necessary. When you import the AngularFirestoreModule it will include the import firebase/firestore behind the scenes. Are you running into a bug due to this?

[marob]

When you import the AngularFirestoreModule it will include the import firebase/firestore behind the scenes.

Maybe it should, but it doesn't.

Are you running into a bug due to this?

Indeed, if the firebase/firestore is not manually imported, it won't work. Hence my PR to explicitly tell to add the import, otherwise, following the steps of the Quickstart fails if you don't pay attention to the code snippets and see the import firebase/firestore in it.

[Splaktar]

It seems like this inclusion of the associated firebase/xlibrary as part of the AngularFireXModule isn't consistent across modules. I've at least noticed this in the documentation. So either the documentation is inconsistent and needs to be updated or both the docs and implementations are inconsistent.

Are there some cases where including the firebase/x import in the AngularFireXModule doesn't work or doesn't make sense?

[mjbates7]

I've also just run into the same issuw trying to use AngularFirestoreModule. I had to explicitly declare import 'firebase/firestore' at the top of my app.module.ts file to make it work on v6 where previously I didn't have to do this.

Angular: 9.0.0
fire: 6.0.0-rc.1

[jamesdaniels]

The developer will need to import the Firebase component themselves in the non-lazy modules; Firestore, Database, and Storage if they're using the AngularFire v6 release candidate. FYI @davideast @Splaktar.

Importing these modules ourselves was breaking in Ivy/ngcc during the Angular v9 release candidates, I have meant to check if the change is ultimately needed in Angular 9.0-stable. It was due to ngcc not understanding the bare import had side-effects. Marking side effects in the library had other negative consequences, so in the meantime yes, this PR is valid.

The "lazy" modules are side-effect free due to their dynamically importing the required Firebase SDK component, making the required API changes for Firestore, Database, and Storage just didn't make the cut for v6. I hope to have lazy versions of these modules in v6.1, which we will then make the default in v7.

Sorry about the radio silence here, I'm back working on AngularFire this week and will be trying to wrap the RC phase and get v6 out the door; including PRs such as this (unless I can revert the change.)

If I'm unable to revert than we will need to make it very explicit in the docs & put it in the upgrade schematic.

[jamesdaniels]

I've confirmed that NGCC does not strip the bare imports & the side-effects remain in place as expected (except for Firestore, for which I have a work around).

I'll be reverting this change in RC.2 (#2347)