-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: Add ESM support #1142
feat: Add ESM support #1142
Conversation
"build-cjs": "NODE_ENV=production BABEL_ENV=cjs babel src --out-dir dist --copy-files --extensions '.js,.jsx,.ts,.tsx'", | ||
"build-declaration": "tsc --project tsconfig.json && tsc-alias -p tsconfig.json", | ||
"build-esm": "NODE_ENV=production BABEL_ENV=esm babel src --out-dir dist/esm --copy-files --extensions '.js,.jsx,.ts,.tsx'", | ||
"build-declaration-esm": "tsc --project tsconfig.json --outDir dist/esm && tsc-alias -p tsconfig.json --outDir dist/esm", |
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 believe there is the need for only one declaration - this mirrors the "types" key on the package.json that I believe is missing as well
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.
Having generated types only in the root folder /dist
doesn't work for imports from /dist/esm
even if the package.json has this set: "types": "dist/types/index.d.ts"
because in the case of ESM import the import path prefix is not / (index)
but /dist/esm
which doesn't match any types path.
I found that the simplest approach would be to create a copy of the types in dist/esm
to have the default TS resolution work for ESM modules.
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 see. Shouldn't we have any types
key tough ?
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 don't think it is required since the generated type path for CJS is /dist/index.d.ts
which matches the imported path @canonical/react-components (index.js)
. Also for ESM, the same logic applies if we use a copy of the types in /dist/esm
.
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.
We can be explicit about it, but it is not required since it is automatically resolved based on the file name.
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.
One minor change
The issue with Percy and iframe has been fixed (#1145) so rebasing this branch on main should fix this problem here. |
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Done
/dist
which can be used by importing fromimport { Button } from "@canonical/react-components/dist/esm"
./dist/esm
to get the type auto resolution working for the ESM modules.