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

Use tsup to build project compatible with CJS and ESM, and validate in CI #258

Closed
wants to merge 6 commits into from

Conversation

sjdemartini
Copy link
Owner

@sjdemartini sjdemartini commented Aug 20, 2024

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 importantly tsup 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:

Some related reading:


With this, npx @arethetypeswrong/cli and npx publint now show as valid (now also part of CI):

npx @arethetypeswrong/cli@0.15.4 --pack

mui-tiptap v1.9.5

Build tools:
- typescript@^5.1.6
- vite@^5.3.1
- tsup@^8.2.4

 No problems found 🌟


┌───────────────────┬──────────────┬────────────────────┐
│                   │ "mui-tiptap" │ "mui-tiptap/icons" │
├───────────────────┼──────────────┼────────────────────┤
│ node10            │ 🟢           │ 🟢                 │
├───────────────────┼──────────────┼────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS)     │ 🟢 (CJS)           │
├───────────────────┼──────────────┼────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)     │ 🟢 (ESM)           │
├───────────────────┼──────────────┼────────────────────┤
│ bundler           │ 🟢           │ 🟢                 │
└───────────────────┴──────────────┴────────────────────┘

whereas the approach in #257 was not :

npx @arethetypeswrong/cli@0.15.4 --pack

mui-tiptap v1.9.5

Build tools:
- typescript@^5.1.6
- vite@^5.3.1

🎭 Import resolved to a CommonJS type declaration file, but an ESM JavaScript file. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md


┌───────────────────┬────────────────────────┬────────────────────────┐
│                   │ "mui-tiptap"           │ "mui-tiptap/icons"     │
├───────────────────┼────────────────────────┼────────────────────────┤
│ node10            │ 🟢                     │ 🟢                     │
├───────────────────┼────────────────────────┼────────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS)               │ 🟢 (CJS)               │
├───────────────────┼────────────────────────┼────────────────────────┤
│ node16 (from ESM) │ 🎭 Masquerading as CJS │ 🎭 Masquerading as CJS │
├───────────────────┼────────────────────────┼────────────────────────┤
│ bundler           │ 🟢                     │ 🟢                     │
└───────────────────┴────────────────────────┴────────────────────────┘

@sjdemartini
Copy link
Owner Author

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 ColorSwatchButton in an external package with this version of mui-tiptap gives:

Warning: React.jsx: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: object.

Check the render method of `ColorSwatchButton`.
    at http://localhost:9000/static/ui/node_modules/.vite/deps/mui-tiptap.js?v=924a118a:6591:25
...

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".
sjdemartini added a commit that referenced this pull request Aug 21, 2024
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).
sjdemartini added a commit that referenced this pull request Aug 21, 2024
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;
```
sjdemartini added a commit that referenced this pull request Aug 21, 2024
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;
```
sjdemartini added a commit that referenced this pull request Aug 21, 2024
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;
```
@sjdemartini
Copy link
Owner Author

Superseded by #259

@sjdemartini
Copy link
Owner Author

Copying notes from #259 and adding more context about why this approach didn't work when I tried it: 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 path rewriting 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

If I force the @mui/icons-material imports to add a file extension via the import renaming, which per https://nodejs.org/api/esm.html#esm_mandatory_file_extensions should be necessary for any package that doesn't have an "exports" field (and @mui/icons-material does not), it breaks with a different problem:

/path/to/mui-tiptap-in-node/node_modules/@mui/icons-material/esm/FormatColorFill.js:3
import createSvgIcon from './utils/createSvgIcon';
^^^^^^

SyntaxError: Cannot use import statement outside a module

since icons-material doesn't in fact use proper ESM syntax internally. It's unclear whether this can be resolved without MUI fixing ESM on their end.

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 this pull request may close these issues.

ESM compatibility - unable to build
1 participant