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

[core] Add exports field and build proper ES modules (no import extensions variant) #821

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Nov 15, 2024

Builds "true" ES modules alongside CommonJS output.

Summary of changes

  • ESM build output is placed under /build/esm
  • CommonJS build output is placed under /build/cjs
  • Per-component package.json files were removed
  • The exports field was added to the package
  • The typesVersion field was added to package.json for compatibility with older tsconfig settings
  • Are The Types Wrong CLI is executed on CI to check if no issues with the package are introduced.

Type definitions

To verify if different methods of module resolution in TypeScript can resolve the exported types, the Are The Types Wrong (ATTW) tool was used. Its reports led to building the type definitions per build output (to avoid errors like False CJS and False ESM.
Another reported issue was the inability to resolve types under the node10 module resolution when the exports field is used. While we don't support Node 10, Typescript uses this resolution strategy by default (see https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md#false-positive-unsupported-file-extension).
To work around this, I introduced the typesVersions field in package.json, as described in https://github.com/andrewbranch/example-subpath-exports-ts-compat/tree/main/examples/node_modules/types-versions-wildcards.

Testing

I created a repo to test the package with different bundlers and plain Node.js: https://github.com/michaldudak/base-ui-bundler-tests. There is an issue with running a Node.js app in ESM mode due to Base UI dependency on @mui/utils. The utils package does not currently conform to the ESM spec and cannot be used without a bundler this way.
As we're planning to move away from it and define our utilities locally (#827), this won't be a problem for long.


This is a variant of #745 that does not require file extensions in import/export specifiers. It uses babel-plugin-add-import-extension to modify JS files and tsc-alias to modify type definitions.

@michaldudak michaldudak added the core Infrastructure work going on behind the scenes label Nov 15, 2024
@michaldudak michaldudak changed the title [core] [core] Add exports field and build proper ES modules (no import extensions variant) [core] Add exports field and build proper ES modules (no import extensions variant) Nov 15, 2024
@mui-bot
Copy link

mui-bot commented Nov 15, 2024

Netlify deploy preview

https://deploy-preview-821--base-ui.netlify.app/

Generated by 🚫 dangerJS against 4e658eb

@Janpot
Copy link
Member

Janpot commented Nov 15, 2024

I'd expected the result after running tsc-alias to be the same regardless of esm or cjs. isn't that the case if you used the same file extension for both?

@michaldudak
Copy link
Member Author

It does look the same in this case, yes (I don't know if that's true for any input, but I haven't found anything that could cause differences so far).

I went the simpler way, not having to deal with copying build .d.ts files across directories, but since running tsc twice adds a few seconds to the build time, I now redid this the right way - building types once to the esm directory, running tsc-alias on the output, and copying them to the cjs directory.

@michaldudak michaldudak marked this pull request as ready for review November 18, 2024 13:43
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 2024
Copy link
Contributor

@vladmoroz vladmoroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The built package looks great

publint is also happy apart from the typos I mentioned

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes scope: code-infra Specific to the code-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants