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

fix(types): move the types condition to the front #273

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jun 2, 2023

I moved types condition to the front. package.json#exports are order-sensitive - they are always matched from the top to the bottom. When a match is found then it should be used and no further matching should occur.

Right now, the current setup works in TypeScript but it's considered a bug and it should not be relied upon, see the thread and the comment here. For that reason, I would like to fix all popular packages that misconfigured their exports this way so the bug can be fixed in TypeScript.

⚠️ this PR focuses solely on fixing "🐛 Used fallback condition" problem but the "🎭 Masquerading as CJS" remains here. You can check the reported errors here

@Andarist Andarist requested a review from favna as a code owner June 2, 2023 07:50
@favna
Copy link
Member

favna commented Jun 2, 2023

I am honoured that Shapeshift is considered a "popular package" for this PR to be made.

I am curious however how we would go about fixing the "🎭 Masquerading as CJS" issue. We bundle for CJS, ESM and the web all at once with tsup (our config) and I would have no idea how we can improve that so we'd fix this issue.

@Andarist
Copy link
Contributor Author

Andarist commented Jun 2, 2023

I am curious however how we would go about fixing the "🎭 Masquerading as CJS" issue. We bundle for CJS, ESM and the web all at once with tsup (our config) and I would have no idea how we can improve that so we'd fix this issue.

By using tsup here you are creating a dual package. I consider this to be a mistake because you are making your package prone to dual package hazard. It's a popular setup though, so 🤷‍♂️ at least you are not alone in this :P

But either way, by shipping a dual package you are shipping your runtime code twice, and both of those copies can be loaded at the same time by the runtime. In such a situation... it kinda only makes sense to ship your declaration files twice as well. This is the only way for your types to reflect 1 to 1 what the runtime can actually see. So for your .js files you need .d.ts, for your .mjs you need .d.mts and for your .cjs you need .d.cts.

All of this is somewhat complicated and tsup doesn't make it any easier (it doesn't handle it out of the box, I believe that there are open issues in their repo about this so you can find them and track them). So I wouldn't be that surprised if you wouldn't like to fix this at the moment.

@favna
Copy link
Member

favna commented Jun 2, 2023

Interesting. Thank you for the explanation. Yeah personally for now I'm a proponent of dual packages until the ecosystem at large starts to move more towards ESM. I'm not too fond of the idea of being the one to alienate a lot of our users.

In particular, as far as Sapphire is concerned is that we have to consider our roots. Those lie in @sapphire/framework, a framework for the (mega) popular DiscordJS Discord bot library so as long as DiscordJS ships both CJS and ESM support, so should we.

There have been polls regarding a move to full ESM and so far there really isn't a consensus yet. DiscordJS also has a pretty big backlog of other tasks to do first so I don't see it happening in the coming years honestly.

Eventually, this package and other Sapphire packages will definitely be ESM only, but that'll be quite some time from now.

@favna favna merged commit 5a3e202 into sapphiredev:main Jun 2, 2023
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.

2 participants