-
Notifications
You must be signed in to change notification settings - Fork 80
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 #230 : CJS / ESM conflict #220
Conversation
Hey @tipy01 👋 thanks for the PR. However, this has to be done carefully (see https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons), so I'll get to this when possible |
Bumps [postcss](https://github.com/postcss/postcss) from 8.4.29 to 8.4.31. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.4.29...8.4.31) --- updated-dependencies: - dependency-name: postcss dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
In last forced commit I:
But I do not use SvelteKit so I guess someone else should test that on a SvelteKit based project. |
This looks good to me. I've dealt with the CJS/ESM stuff a ton, so am pretty confident in this PR. I'll note that introducing |
@tipy01 thanks for the update! I will get to test this this weekend and hopefully merge it soon. Following what @benmccann suggested, I think having the exports is the correct way forward too. I'm gonna check if bumping the |
I found a stackoverflow thread about that. |
"import": "./dist/runtime.js", | ||
"require": "./dist/runtime.cjs" |
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'd personally make it just:
"import": "./dist/runtime.js", | |
"require": "./dist/runtime.cjs" | |
"default": "./dist/runtime.js" |
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.
Yes and it would work also just adding the "default": "./dist/runtime.js"
line because it is just a fallback. All this stuff is documented here.
I personally do not need of cjs so I do not care on if this package is a "Dual CommonJS/ES module package" or just esm.
@kaisermann will split this PR and then he will decide on keeping or not the cjs.
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 think we can be esm-only from v4 onwards, assuming 3.7.5
will be working as expected.
Gonna merge this onto the |
Before submitting the PR, please make sure you do the following
npm run lint
!)Tests
npm test
oryarn test