Skip to content
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

Expose consistent dist files across packages #3445

Closed
Tracked by #2964
nolanlawson opened this issue Apr 5, 2023 · 2 comments · Fixed by #3585
Closed
Tracked by #2964

Expose consistent dist files across packages #3445

nolanlawson opened this issue Apr 5, 2023 · 2 comments · Fixed by #3585

Comments

@nolanlawson
Copy link
Collaborator

nolanlawson commented Apr 5, 2023

Original description

Currently, each package in the monorepo has its own build script. Sometimes we use tsc, other times we use rollup. Each one has its own rollup.config.js. The output is sometimes dist, sometimes types, and the output dist file names are occasionally different.

Instead, we should standardize our output:

  • Use Rollup for everything
  • Have one Rollup config file (or one that is simply exported by other Rollup config files)
  • Always output dist/ and types/ directories in the same way – dist/index.js for ESM, dist/index.cjs for CommonJS
  • Have the same "main"/"module"/"typings" keys in every package.json

This is technically a breaking change, since we would be changing the output format of our packages, which people might be relying on. (E.g. import 'some-package/dist/deep/dependency/here.js')

As of #3456, we now have a consistent build process across packages. What's inconsistent is our dist files, for backwards-compat reasons:

// TODO [#3445]: this purely exists for backwards compatibility, to avoid breaking code like this:
//
// require('@lwc/synthetic-shadow/dist/synthetic-shadow.js')
// require('@lwc/engine-server/dist/engine-server.js')
// require('@lwc/compiler/dist/commonjs/transformers/transformer')
//
// Feel free to delete this entire plugin once we can safely release this breaking change.
function backwardsCompatDistPlugin() {
const packageNamesToExtraDistFiles = {
'@lwc/compiler': {
'index.cjs.js': 'dist/commonjs/transformers/transformer.js',
},
'@lwc/engine-dom': {
'index.js': 'dist/engine-dom.js',
},
'@lwc/engine-server': {
'index.js': 'dist/engine-server.js',
},
'@lwc/synthetic-shadow': {
'index.js': 'dist/synthetic-shadow.js',
},
'@lwc/wire-service': {
'index.js': 'dist/wire-service.js',
},
};

We should drop these in LWC v3.0.0.

@nolanlawson
Copy link
Collaborator Author

I took a stab at this (nolanlawson@2d866c9) but it's actually incredibly tricky to get right given issues with ESM vs CommonJS:

  1. Rollup may load stuff from rollup.config.js as CommonJS, even though it's authored in an ESM format
  2. Jest gets confused about default exports vs named exports – best is to just never use default exports like we do in @lwc/template-compiler, @lwc/style-compiler, etc.
  3. Using the "exports" syntax in package.json with conditional exports causes the ESM version to be preferred, leading to the Rollup problem mentioned above
  4. Using the "module" keyword in a package causes Jest to use that, leading to the Jest problem mentioned above

I can't see any way forward except either 1) not exposing ESM for Node-only packages like @lwc/compiler, or 2) not exporting a default module for such packages.

Of course, all of these problems would go away if we were a pure-ESM package, but given Jest/Rollup/etc issues, it feels too early to force that on our consumers.

@nolanlawson
Copy link
Collaborator Author

The solution I decided on in #3456 was to not expose "exports", and to fix Jest by using named exports.

I also kept some dist/ files for backwards compat, but we can hopefully address that in LWC v3.0.0.

@nolanlawson nolanlawson changed the title Use consistent build process for all packages Expose consistent dist files across packages May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant