Add exports field with subpaths to firebase-exp/package.json #4767
Conversation
hsubox76
requested review from
avolkovi,
Feiyang1,
hiranya911,
sam-gc and
yuchenshi
as code owners
|
| "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.
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.
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.
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.
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.
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.
Should we just update the package.json?
There was a problem hiding this comment.
Oh, I forgot it sources this from a specialized exp-only package.json. Yes, will fix.
hsubox76
changed the title
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.