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

refactor: switch to split cjs and esm builds and fully build with tsup #697

Merged
merged 3 commits into from
Dec 2, 2023

Conversation

favna
Copy link
Member

@favna favna commented Dec 1, 2023

I asked about this before in the server and I figured I'd just try it out, create a PR, and hear your thoughts on whether we merge this or not.

Zip of the new dist folder after running prepack:

new-dist.zip


Are the types wrong report before:

🎭 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

🐛 Import resolved to types through a conditional package.json export, but only after failing to resolve through an earlier condition. This behavior is a TypeScript bug (https://github.com/microsoft/TypeScript/issues/50762). It may misrepresent the runtime behavior of this import and should not be relied upon. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FallbackCondition.md


┌───────────────────┬────────────────────────────┐
│                   │ "@sapphire/framework"      │
├───────────────────┼────────────────────────────┤
│ node10            │ 🟢                         │
├───────────────────┼────────────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS)                   │
├───────────────────┼────────────────────────────┤
│ node16 (from ESM) │ 🎭 Masquerading as CJS     │
│                   │ 🐛 Used fallback condition │
├───────────────────┼────────────────────────────┤
│ bundler           │ 🐛 Used fallback condition │
└───────────────────┴────────────────────────────┘

Are the types wrong report after:

 No problems found 🌟


┌───────────────────┬───────────────────────┐
│                   │ "@sapphire/framework" │
├───────────────────┼───────────────────────┤
│ node10            │ 🟢                    │
├───────────────────┼───────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS)              │
├───────────────────┼───────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)              │
├───────────────────┼───────────────────────┤
│ bundler           │ 🟢                    │
└───────────────────┴───────────────────────┘

@favna favna self-assigned this Dec 1, 2023
package.json Show resolved Hide resolved
@favna favna merged commit 2502abb into main Dec 2, 2023
7 checks passed
@favna favna deleted the refactor/switch-to-cjs-esm-split-and-full-tsup branch December 2, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants