Point browser field to esm build #3932
Conversation
Feiyang1
requested review from
alikn,
avolkovi,
bojeil-google,
davideast,
erikeldridge,
hiranya911,
hsubox76,
jamesdaniels,
jsdt,
sam-gc,
schmidt-sebastian and
zijianjoy
as code owners
|
I think you have to change this line: to point to pkg.main or else you make a weird corrupted esm file which is causing the CI build to fail.
There could possibly be a similar issue in some of the other rollup config files. |
Good catch. Updated |
🦋 Changeset detectedLatest commit: 1cf00d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check
|
Size Analysis ReportAffected ProductsNo changes between base commit (b6a9bf0) and head commit (ae82d9a). Test Logs
|
Binary Size ReportAffected SDKs
Test Logs
|
| import { _FirebaseApp } from '@firebase/app-types/private'; | ||
| import { FirebaseAuthInternal } from '@firebase/auth-interop-types'; | ||
| import * as request from 'request'; | ||
| import { base64 } from '@firebase/util'; | ||
| import { setLogLevel, LogLevel } from '@firebase/logger'; | ||
| import { Component, ComponentType } from '@firebase/component'; | ||
|
|
||
| export { database, firestore } from 'firebase'; | ||
| const { firestore, database } = firebase; | ||
| export { firestore, database }; |
There was a problem hiding this comment.
@samtstern @yuchenshi
Meant to ask you before merging the PR - How does developer use these two exports? Do they use the typings from them?
They won't get typings after this change because firestore and database are not namespaces anymore.
There was a problem hiding this comment.
I believe they're used to provide access to Firestore / RTDB classes like this:
https://github.com/firebase/firebase-js-sdk/blob/master/packages/rules-unit-testing/test/database.test.ts#L313
If those tests still pass I think we're fine? To be honest I don't quite understand namespaces.
There was a problem hiding this comment.
Yeah, that will continue to work. We are good. Thanks for confirming.
Rationale
Make Firebase work nicely with Angular10.
Angular10 complains loudly if the browser field points to cjs build. And it's hard for developer to change build configuration which is encapsulated by ngcc
See FR: Make Firebase SDK compatible with Angular 10 #3315
Address the incompatibility issue between our typings and esm builds
Our typings are modeled after the cjs module format, using
export = firebase, so we ask developers to use namespace import to import firebase, like so:import * as firebase from ‘firebase/app’, but it’s not compatible with our esm builds which only have a default export.If you manually configure the bundler to use our esm build (e.g. using the
modulefield or theesm2017field), you will get an error. For example, you will get the following error with webpack:"export 'initializeApp' (imported as 'firebase') was not found in 'firebase/app'This change allows developers to use any of our builds without resorting to hacks.