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.

Sign up for GitHub

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

Jump to bottom

fix #230 : CJS / ESM conflict #220

Merged
merged 2 commits into from
Oct 14, 2023
Merged

fix #230 : CJS / ESM conflict #220

merged 2 commits into from
Oct 14, 2023

Conversation

tipy01
Copy link
Contributor

@tipy01 tipy01 commented Jul 11, 2023 •
edited
Loading

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test
@kaisermann
Copy link
Owner

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
@dependabot
chore(deps-dev): bump postcss from 8.4.29 to 8.4.31
0d0415f 
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>
@tipy01
Copy link
Contributor Author

tipy01 commented Oct 12, 2023

In last forced commit I:

  • rebased to main
  • use of .mjs and .cjs extension instead of .esm.js and .cjs.js (doing such change make my both Svelte3/ts-nodeCjs and Svelte4/ts-nodeEsm projects unit tests and build working properly, better compared with my Allow unit tests with ts-node esm #219 issue comment).
  • upgraded the intl-messageformat from 9.13.0 to 10.5.3 to target the CJS / ESM conflict #230 issue

But I do not use SvelteKit so I guess someone else should test that on a SvelteKit based project.
@tipy01 tipy01 changed the title fix #219: allow unit tests with ts-node esm Oct 12, 2023
@tipy01 tipy01 mentioned this pull request Oct 12, 2023
Closed
benmccann
benmccann approved these changes Oct 12, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@benmccann
Copy link

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 exports is encouraged but is also technically a breaking change at it stops people from being able to access non-exported files. So it's probably good practice to bump the major version when merging this PR. And if you're doing that, I'd recommend also dropping the CJS version at the same time
@kaisermann
Copy link
Owner

@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 intl-messageformat is enough to fix it for the current major. If it is, I will release a version with only that. Then I will release the new major with the rest of the changes.
@tipy01
Copy link
Contributor Author

tipy01 commented Oct 12, 2023

I'm gonna check if bumping the intl-messageformat is enough to fix it ...

I found a stackoverflow thread about that.
benmccann
benmccann reviewed Oct 12, 2023
View reviewed changes
package.json
Comment on lines +8 to +9
"import": "./dist/runtime.js",
"require": "./dist/runtime.cjs"
Copy link

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:

Suggested change
"import": "./dist/runtime.js",
"require": "./dist/runtime.cjs"
"default": "./dist/runtime.js"
Copy link
Contributor Author

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.
Copy link
Owner

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.
@kaisermann kaisermann changed the base branch from main to esm-fix October 14, 2023 09:29
@kaisermann kaisermann changed the base branch from esm-fix to main October 14, 2023 09:31
@kaisermann
Copy link
Owner

kaisermann commented Oct 14, 2023 •
edited
Loading

Gonna merge this onto the v4 branch and work from there 🙏 thanks for all the help

#238
@kaisermann kaisermann changed the base branch from main to v4 October 14, 2023 10:06
@kaisermann kaisermann merged commit d54ee67 into kaisermann:v4 Oct 14, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@tipy01 @kaisermann @benmccann