-
Notifications
You must be signed in to change notification settings - Fork 48
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
Use tsup to build project compatible with CJS and ESM, and validate in CI #258
Conversation
oof, it turns out this breaks in a non-obvious way in consuming packages due to mui/material-ui#35233 (specifically what's described in mui/material-ui#35233). i.e., material-ui does not properly support imports of icons at their subpaths in an ESM context, same as https://stackoverflow.com/q/72008357/4543977. e.g. trying to use the
Will try some workarounds. |
With this, `npx @arethetypeswrong/cli` and `npx publint` now show as valid, whereas the approach in #257 was not.
Per recommendation from publint. https://publint.dev/rules#use_type > The package does not specify the "type" field. NodeJS may attempt to detect the package type causing a small performance hit. Consider adding "type": "commonjs".
348f606
to
8cdcfdb
Compare
Both `@mui/icons-material` (mui/material-ui#35233) and `lodash` (lodash/lodash#5107) have problems being imported in a consuming package when using ESM. The workarounds attempted in #258 almost seemed to work (didn't break a downstream bundled package using Vite), but still caused problems for the original example node application https://codesandbox.io/p/devbox/pensive-volhard-hyhtls, with errors like: ``` Error: Cannot find module '/path/to/mui-tiptap-in-node/node_modules/@mui/icons-material/esm/FormatColorFill ``` This approach is inspired by what tss-react does https://github.com/garronej/tss-react/blob/f5351e42e33f35f18415cfc1ffc6b08eb8ce4d25/package.json (e.g. see here garronej/tss-react@4699702 and garronej/tss-react#164).
Should resolve #256 Both `@mui/icons-material` (mui/material-ui#35233) and `lodash` (lodash/lodash#5107) have problems being imported in a consuming package when using ESM. The workarounds attempted in #258 almost seemed to work (didn't break a downstream bundled package using Vite), but still caused problems for the original example node application https://codesandbox.io/p/devbox/pensive-volhard-hyhtls, with errors like: ``` Error: Cannot find module '/path/to/mui-tiptap-in-node/node_modules/@mui/icons-material/esm/FormatColorFill ``` This approach is inspired by what tss-react does https://github.com/garronej/tss-react/blob/f5351e42e33f35f18415cfc1ffc6b08eb8ce4d25/package.json (e.g. see here garronej/tss-react@4699702 and garronej/tss-react#164). With this change, this code now works in the node context (though slightly odd): ``` import pkg from "mui-tiptap"; const { FontSize, HeadingWithAnchor, ResizableImage, TableImproved } = pkg; ```
Should resolve #256. Both `@mui/icons-material` (mui/material-ui#35233) and `lodash` (lodash/lodash#5107) have problems being imported in a consuming package when using ESM. The workarounds attempted in #258 almost seemed to work (didn't break a downstream bundled package using Vite), but still caused problems for the original example node application https://codesandbox.io/p/devbox/pensive-volhard-hyhtls, with errors like: ``` Error: Cannot find module '/path/to/mui-tiptap-in-node/node_modules/@mui/icons-material/esm/FormatColorFill ``` This approach is inspired by what tss-react does https://github.com/garronej/tss-react/blob/f5351e42e33f35f18415cfc1ffc6b08eb8ce4d25/package.json (e.g. see here garronej/tss-react@4699702 and garronej/tss-react#164). With this change, this code now works in the node context (though slightly odd): ``` import pkg from "mui-tiptap"; const { FontSize, HeadingWithAnchor, ResizableImage, TableImproved } = pkg; ```
Should resolve #256. Both `@mui/icons-material` (mui/material-ui#35233) and `lodash` (lodash/lodash#5107) have problems being imported in a consuming package when using ESM. The workarounds attempted in #258 almost seemed to work (didn't break a downstream bundled package using Vite), but still caused problems for the original example node application https://codesandbox.io/p/devbox/pensive-volhard-hyhtls, with errors like: ``` Error: Cannot find module '/path/to/mui-tiptap-in-node/node_modules/@mui/icons-material/esm/FormatColorFill ``` This approach is inspired by what tss-react does https://github.com/garronej/tss-react/blob/f5351e42e33f35f18415cfc1ffc6b08eb8ce4d25/package.json (e.g. see here garronej/tss-react@4699702 and garronej/tss-react#164). With this change, this code now works in the node context (though slightly odd): ``` import pkg from "mui-tiptap"; const { FontSize, HeadingWithAnchor, ResizableImage, TableImproved } = pkg; ```
Superseded by #259 |
Copying notes from #259 and adding more context about why this approach didn't work when I tried it: Both
If I force the
since |
Should resolve #256.
This changes our packaging approach to using bundling via tsup, whereas to date we've only used
tsc
to generate separate JS files. It seems bundling (but not including external dependencies) is standard from most major packages I audited, and importantlytsup
greatly simplifies how the build files are generated, ensuring CJS and ESM compatibility, in a way that was otherwise quite annoying.This also now adds source maps, which will likely be useful for users of mui-tiptap.
Other options considered:
tsup
worked much more easily, such as excluding external dependencies by default, and building for CJS specifically rather than UMD."type": "module"
and "NodeNext"moduleResolution
with typescript, which means all imports within mui-tiptap have to use.js
extensions, etc., which is quite annoying. I'd prefer minimal churn here.Some related reading:
With this,
npx @arethetypeswrong/cli
andnpx publint
now show as valid (now also part of CI):whereas the approach in #257 was not :