Add Firestore, initializeFirestore, getFirestore to Firestore lite #3108
Conversation
| import firebase from '@firebase/app'; | ||
| import { FirebaseNamespace } from '@firebase/app-types'; |
There was a problem hiding this comment.
Are you planning to make Firestore lite work with the existing SDK? If not, you should take dependency on @firebase/app-next and @firebase/app-types-next instead.
There was a problem hiding this comment.
Using @firebase/app-next to register components:
import { _registerComponent } from '@firebase/app-next';
_registerComponent(new Component(...))
There was a problem hiding this comment.
I am not planning on making this work with the existing SDK. Apart from added confusion, there is no real benefit as far as I can tell.
| @@ -0,0 +1,104 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2017 Google LLC | |||
| import { FirebaseApp } from '@firebase/app-types'; | ||
| import { FirebaseService } from '@firebase/app-types/private'; |
There was a problem hiding this comment.
Should depend on @firebase/app-types-exp instead.
It currently doesn't define a FirebaseService interface, I'm adding it in https://github.com/firebase/firebase-js-sdk/pull/3112/files
There was a problem hiding this comment.
Added TODO and removed FirebaseService.
packages/firestore/package.json
Outdated
| "prepare": "yarn build" | ||
| }, | ||
| "lite": "dist/lite/index.node.cjs.js", |
There was a problem hiding this comment.
Should lite has its own package.json, so people can import using import {...} from '@firebase/firestore/lite'?
| json({ preferConst: true }) | ||
| ]; | ||
|
|
||
| const nodeBuilds = [ |
There was a problem hiding this comment.
I guess you will add browser builds eventually?
There was a problem hiding this comment.
This slows down development for now. Added TODO.
|
@Feiyang1 The binary size presubmit requires me to add the Lite build to |
Binary Size ReportAffected SDKsTest Logs
|
…irebase-js-sdk into mrschmidt/initializefirestore
| { apiKey: 'fake-api-key', projectId: 'test-project' }, | ||
| 'test-app-initializeFirestore' | ||
| ); | ||
| initializeFirestore(app, { host: 'localhost', ssl: false }); |
There was a problem hiding this comment.
Did you forget to assert that the setting object on the Firestore instance equals the input?
There was a problem hiding this comment.
I only meant to make sure that this doesn't throw. There is no API to read the settings. I added an instanceOf check to make this more like a proper test.
packages/firestore/package.json
Outdated
| "@firebase/app-types": "0.x" | ||
| "@firebase/app-exp": "0.x", | ||
| "@firebase/app-types": "0.x", | ||
| "@firebase/app-types-ext": "0.x" |
There was a problem hiding this comment.
Should not add -exp packages in dependencies or peerDependencies, because they don't really exist on npm. For now, can you just suppress the eslint errors?
packages/firestore/package.json
Outdated
| "build:deps": "lerna run --scope @firebase/'{app,firestore}' --include-dependencies build", | ||
| "build:console": "node tools/console.build.js", | ||
| "build:exp": "rollup -c rollup.config.exp.js", | ||
| "build:exp": "rollup -c rollup.config.exp.js", | ||
| "build:lite": "lerna run --scope @firebase/app-exp --include-dependencies build && rollup -c rollup.config.lite.js", |
There was a problem hiding this comment.
You may want to add a new script build:lite:deps that builds @firebase/app-exp and deps, while keeping this script simply executing rollup. During dev, you execute build:lite:deps once to compile all deps, then use build:lite for quick firestore rebuild.
There was a problem hiding this comment.
I assume this is not applicable if I use devDependencies? At least the PR builds without it. I am learning by doing here :)
schmidt-sebastian
force-pushed
the
mrschmidt/initializefirestore
branch
from
e25903a to
804fe94
Compare
This adds the wiring for Firestore, initializeFirestore, getFirestore in the firestore/lite build.