Firestore exp release script #3444
Conversation
This reverts commit 5837dab.
💥 No ChangesetLatest commit: 19fd38e Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Binary Size ReportAffected SDKs
Test Logs
|
| treeshake: { | ||
| moduleSideEffects: false | ||
| } |
There was a problem hiding this comment.
This made no difference in my size testing so I removed it.
There was a problem hiding this comment.
You wouldn't need it normally.
I use it to remove an empty import (import '@firebase/app-types') caused byimportPathTransformer.
packages/firestore/exp/package.json
Outdated
| @@ -1,8 +1,10 @@ | |||
| { | |||
| "name": "@firebase/firestore/exp", | |||
| "description": "A tree-shakeable version of the Firestore SDK", | |||
| "main": "../dist/exp/index.node.umd.js", | |||
| "main": "../dist/exp/index.node.esm2017.js", | |||
There was a problem hiding this comment.
Should we update these names to match the ones you defined in firestore-exp? Alternatively, can we just drop this?
There was a problem hiding this comment.
I need to revert this. What do you mean the ones you defined in firestore-exp?
There was a problem hiding this comment.
We should use the same names in packages-exp/firestore/package.json and packages/firestore/exp/package.json
But it seems like we could just remove packages/firestore/exp/package.json
There was a problem hiding this comment.
There is no packages-exp/firestore. Do you mean packages-exp/firebase-exp/firestore/package.json?
I need packages/firestore/exp/package.json for the location of the binary files for exp, so I can use them to update the fields in packages/firestore/package.json.
The exp release process is hacky and might look confusing. Hopefully we just need it for a short period of time.
| "main": "dist/index.cjs.js", | ||
| "browser": "dist/index.esm.js", | ||
| "module": "dist/index.esm.js", | ||
| "typings": "dist/firestore/lite/index.d.ts" |
There was a problem hiding this comment.
Are these values used at all? Looks like the release script replaces them. Might make sense to use dummy values if that is indeed the case.
There was a problem hiding this comment.
Yes. They are used when you do import {...} from 'firebase/firestore/lite'
scripts/utils.ts
Outdated
|
|
||
| export async function readPackageJson(packagePath: string) { | ||
| /** | ||
| * Nout using require here because require caches the file |
| // Prepare @firebase/firestore, so scripts/exp/release.ts can be used to release it | ||
| export async function prepare() { | ||
| // Update package.json | ||
| const packageJson = await readPackageJson(packagePath); |
There was a problem hiding this comment.
I don't fully follow this. Maybe that's ok, maybe we need some comments.
This seems to create a new package.json based on expPackageJson, but it seems to use the dependencies and the name from the main Firestore package. If that understanding is correct, then I think we are not using the right name for firestore/lite. We should also consider using different dependencies so that we don't force webchannel-wrapper on users that don't use a bundler for their code.
There was a problem hiding this comment.
I added some comments. PTAL
Why do you think we are not using the right name for firestore/lite? It will be accessible by @firebase/firestore/lite and firebase/firestore/lite. Is it incorrect?
How do we force webchannel-wrapper on users that don't use a bundler for their code?
Are you talking about people using CDN scripts? We will build firebase-firestore-lite.js in firebase, which should not contain webchannel-wrapper.
I believe firebase/exp has the same dependencies as we currently have, what different dependencies should it have?
There was a problem hiding this comment.
"@firebase/firestore/lite" and "firebase/firestore/lite" is correct.
Re: wenchannel-wrapper. It would be nice if users can develop against "firebase/firestore/lite" without depending on WebChannelWrapper. If we continue to ask users to just install the "firebase" package then this point is moot though.
Thanks for the comment update.
There was a problem hiding this comment.
I see. We won't be able to do that unless we put firestore/lite into its own package. IIUC, it's only an issue for Nodejs apps, right? E.g. the ones running in serverless environments with limited storage. I don't think it matters to Web apps since unused deps will just sit in node_modules, not in the compiled app code.
| @@ -22,7 +22,10 @@ var sourcemaps = require('gulp-sourcemaps'); | |||
| const OUTPUT_FILE = 'firebase.js'; | |||
| const pkgJson = require('./package.json'); | |||
| const files = [ | |||
| ...pkgJson.components.map(component => `firebase-${component}.js`) | |||
| ...pkgJson.components.map(component => { | |||
There was a problem hiding this comment.
I guess it was there before and it's not very important but [...array] seems unnecessary when there's only one item, it could just be array
| @@ -42,7 +43,8 @@ const nodePlugins = [ | |||
| } | |||
| }, | |||
| clean: true, | |||
| transformers: removeAssertTransformer | |||
| abortOnError: false, | |||
| transformers: [...removeAssertTransformer, importPathTransformer] | |||
There was a problem hiding this comment.
Can this and removeAssertAndPrefixInternalTransformer be plural ("transformers") so the spread makes sense?
No description provided.