RFC: Standard package output folders#17298
RFC: Standard package output folders#17298dzearing wants to merge 8 commits intomicrosoft:masterfrom dzearing:rfc/standard-folders
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f0a62f4:
|
ecraig12345
left a comment
ecraig12345
left a comment
There was a problem hiding this comment.
Definitely agree that documented standards are good, but I'm less certain about the importance of being able to tell just from the folder name what the output type is (though not totally opposed to the idea). So I'm curious what others think.
Would also be good to clarify if this is just for v8 packages, or also for converged.
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d9fb5e3ce8dfdf0f6fdb5dee945a29243ec85dc9 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
| - `/lib` - esm (as we've had for a long time) | ||
| - `/lib-commonjs` - commonjs (only needed while Node <= 13.2.0 is supported) | ||
| - `/lib-amd` - amd (hopefully we can drop someday) | ||
| - `/dist` - bundles and static content |
There was a problem hiding this comment.
My personal preference is to have lib-esm or lib-es as it obviously says what is in a directory.
Another thing that we discussed some time ago to have a single dist folder. For example:
/dist/esm- esm (as we've had for a long time)/dist/commonjs- commonjs (only needed while Node <= 13.2.0 is supported)/dist/amd- amd (hopefully we can drop someday)/dist/assets- bundles and static content
It will have the same props&cons as an alternative proposal, but it will be easier to ignore a single entry in .gitignore/IDE 🐱
There was a problem hiding this comment.
If we want to change things, I agree this would be even cleaner
There was a problem hiding this comment.
Also I could behind the lib-esm approach. It's more explicit, and doesn't favor a particular output. This means we can add more output flavors in the future (the react-native output formats would be the first which come to mind) without the akwardness of picking one as the "true" output flavor.
My primary concern changing now is to avoid destabilizing partners, as Elizabeth mentioned below. My recommendation would be to stick with the current output folders in the writeup, fix the node packages only to stay inline with other packages (since this was a recent change.)
Then in the converged packages, we could start adopting the dist/* layout now. Later in v9+, change everything to be inline with the dist approach.
Is this reasonable/ideal? Concerns?
There was a problem hiding this comment.
My primary concern changing now is to avoid destabilizing partners, as Elizabeth mentioned below. My recommendation would be to stick with the current output folders in the writeup, fix the node packages only to stay inline with other packages (since this was a recent change.)
Then in the converged packages, we could start adopting the dist/* layout now. Later in v9+, change everything to be inline with the dist approach.
Is this reasonable/ideal? Concerns?
No concerns, make sense for me 👍
Should we update a proposed solution to include v8 (as it stays currently) and converged proposal (for example, /dist layout, or another proposal) in this RFC? Or have an another one?
There was a problem hiding this comment.
I think it's simple enough that we could cover both v8 and converged in this RFC.
Though the part we didn't totally decide on for v8 is whether lib must be ESM for all packages, even ones where in reality we only need commonjs.
|
Just remembered another thing to consider/make clear, for our main version 8 packages (the ones that run in the browser) we probably shouldn't change the output structure at this point. We've tried to make output structure changes within a major release in the past, and it caused problems for enough people that it had to be reverted (@jdhuntington just reminded me of this). So any changes resulting from this proposal would need to be limited to:
|
| * immer - Uses only `dist` and rolls up a separate `esm` bundle. | ||
| * react - Commonjs is in `cjs`, there's also a `umd` folder for bundles. | ||
| * redux - Uses `es` for esm, `lib` for commonjs. | ||
| * styled-components - Only `dist` with `s-c.esm.js` and `s-c.cjs.js`. They also emit a react-native `s-c.native.js` flavor. |
There was a problem hiding this comment.
important thing to answer - is this RFC for both legacy and converged?
I'd follow google/preact crew way of shipping libraries. They have plenty of experience in both building smallest possible libraries and way of shipping those to consumers.
For inspiration -> https://github.com/developit/microbundle
Convergence output:
- one file rollup bundles (3 types only)
- shipping source is discouraged as you're allowing consumers to use private API's (I understand this was allowed in v0/v7,8 - I recommend to fix this in v9)
"main": "./dist/foo.umd.js", // legacy UMD output (for Node & CDN use)
"module": "./dist/foo.module.js", // legacy ES Modules output (for bundlers)
"exports": "./dist/foo.modern.js", // modern ES2017 output
Pros:
- smallest possible JS stored in registry === faster installs, less "garbage" in node modules
- rollup-ed files faster to parse by bundlers instead of parsing and bundling N modules
- esbuild can be enabled without any further modification
- no exposure of private APIs
- only 1 output instead of 2 for "legacy consumers" (cjs/amd -> umd)
Cons:
- might not work for legacy (v0,7,8) as they consume private apis/nested paths
There was a problem hiding this comment.
regarding dist/, I definitely agree with following suggestion/change:
I briefly mentioned in on of my PR's regarding better DX - TS path aliases, but it got lost within changes...
I'd recommend to have only one dist, coverage(out of topic) folder in root of monorepo
/coverage
/packages/
/react-button
/dist
/packages/
/react-button
| - index.umd.js
| - index.module.js
| - index.modern.js
| - index.d.ts
| - Readme.md
| - License.md
| - package.json // normalized package.json - contains only important data (deps/peerDeps/license,etc)- no scripts/devDeps and other "baggage"
/packages
/react-button
/apps
/todo-app
Pros:
- source code is not being mismatched with generated code
- all generated assets live under one roof
- faster cleanup
- ease of processing
Cons:
- might not work for v0,7,8 as they are not leveraging TS path aliases - additional work would be needed
There was a problem hiding this comment.
important thing to answer - is this RFC for both legacy and converged?
Any nontrivial changes will have to be for converged only (the RFC needs to be updated to mention this). #17298 (comment)
There was a problem hiding this comment.
@Hotell I think 1 dist folder with rollup bundles makes tons of sense for small libraries. With converged packages where they're more single purposed, for example. It is also a very good tool for avoiding deeply nested imports; and as you mentioned, we would probably discourage including build output completely. (There are a few scenarios where it may be useful, like documentation.) Would be nice to roll up the typings as well.
It breaks down in monolithic suite packages that re-export a lot of things, because of a variety of reasons. More to parse unnecessarily, more prone to side effect problems, bundle graph doesn't show enough detail.
So I could get behind this for the converged smaller-purpose packages.
There was a problem hiding this comment.
What about this:
Legacy: we keep the same. lib (esm), lib-commonjs (commonjs), amd (amd)
Converged:
dist/
react-button.esm.js
react-button.d.ts
react-button.commonjs.js
react-button.umd.js
@layershifter @ecraig12345 @JustSlone thoughts?
|
So, given the open conversations. I'd like to suggest "Let's Do Both". For v8 - take the easy no-break fix For vNext: - follow the single |
|
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |
msft-fluent-ui-bot
added
the
Resolution: Soft Close
|
Huh it says "The repository that submitted this pull request has been deleted"?? So I guess if we want this back someone will have to make a new copy. |
8 participants
Standardize javascript package output folders for better predictability and clarity over what folder contains what format.
lib/*- ESMlib-commonjs/*- CommonJSlib-amd/*- AMDdist/*- bundles and static contentThis isn't really a change from what we have, except for projects built with the
build:commonJSOnlytask (wasn't being used until recently) which drops commonjs inlib(probably by accident, because we didn't document the convention.) For everything else this has been the convention with the Just tooling, and with the web-build-tools tooling prior to that. The last time we changed it was when ESM became officially supported.I did include an alternative that seems a little more inline with the output folder naming of a few OSS libraries, but I don't think the work involved and potential for impact on customers merits major deviation. It creates unexpected confusion when we shift the package structure.
Recent conversation with @ecraig12345 and @JustSlone led to formalizing this detail as an RFC.