-
Notifications
You must be signed in to change notification settings - Fork 890
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
Add exports field with subpaths to firebase-exp/package.json #4767
Conversation
|
"exports": { | ||
"main": "./dist/index.cjs.js", | ||
"esm5": "./dist/index.esm.js", | ||
"browser": "./dist/index.esm2017.js", | ||
"module": "./dist/index.esm2017.js", | ||
"node": "./dist/index.cjs.js", | ||
"default": "./dist/index.esm2017.js" | ||
}, |
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.
Does bundler read these fields? I thought we are only using exports
to specify submodules, so we only need to define it for packages that has submodules, e.g. only firebase-exp
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'm not sure, webpack seems to look at a lot of possible fields https://webpack.js.org/guides/package-exports and we don't distinguish between browser/module/react-native/etc in the minimal firebase-exp entry points so if you had an app that cared about the difference it would only find them in the individual @firebase
packages entry points.
"browser": "dist/index.esm2017.js", | ||
"module": "dist/index.esm2017.js", |
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.
Wonder if we don't want to change compat packages, but I guess it's fine since it's going to be a major release
"browser": "./dist/index.esm2017.js", | ||
"module": "./dist/index.esm2017.js", | ||
"node": "./dist/index.cjs.js", | ||
"default": "./dist/index.esm2017.js" |
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.
Should we assign a browser build to default? IIUC, default
is used by Nodejs if nothing else matches, so should it be a node build? or should we just leave it since there is a node
field?
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.
The node examples always put a browser bundle in the default field. I think they assume any Node app will pick up what's in the Node field.
// Set esm2017 as default. | ||
// Delete esm2017 field. | ||
// Add esm5 field. | ||
packageJson.module = expPackageJson.esm2017.replace('../', ''); |
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.
Should we just update the package.json?
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.
Oh, I forgot it sources this from a specialized exp-only package.json. Yes, will fix.
Binary Size ReportAffected SDKsNo changes between base commit (3a18640) and head commit (a894d56). Test Logs
|
Background for exports: https://nodejs.org/api/packages.html#packages_conditional_exports
It's also recognized by webpack and other bundlers so we can't assume it's only consumed by Node applications. Requested here.
Only adding exports to the main package.json for now. We will do the other packages later if there's a need.