-
Notifications
You must be signed in to change notification settings - Fork 982
Unify Pipelines and Firestore classic in CDN and g3 builds #9323
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
base: console
Are you sure you want to change the base?
Conversation
|
Vertex AI Mock Responses Check
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
…t firebase-firestore-pipelines.js contains both classic and pipelines APIs.
… cdn.ts to packages/firebase/firestore/pipelines
…wser unit testing.
| }) | ||
| }), | ||
|
|
||
| declareModuleReplacePlugin() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious why this plugin is necessary. Can you add comments explaining the problem this solves?
| // The regex we created earlier | ||
| const moduleToReplace = | ||
| /declare module '\.\/\S+' \{\s+interface Firestore \{\s+pipeline\(\): PipelineSource<Pipeline>;\s+}\s*}/gm; | ||
|
|
||
| // What to replace it with (an empty string to remove it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments seem out of date. Can you update them?
| ]; | ||
|
|
||
| // TODO - update the implementation to match all content in the declare module block. | ||
| function declareModuleReplacePlugin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All our custom rollup plugins exist in scripts/build. Can you move this plugin to a new file in that directory?
| public asyncQueue: AsyncQueue, | ||
| private databaseInfo: DatabaseInfo, | ||
| /** | ||
| * @internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think @internal is necessary here, since FirestoreClient is not a public API.
For CDN and G3 builds, we will output two bundles:
pipelines).