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(package.json): add export map #652

Merged
merged 5 commits into from
Jun 23, 2023

Conversation

christjt
Copy link
Contributor

@christjt christjt commented May 5, 2023

Running ESM in jest tests (node) isn't working properly with this lib yielding this error:
image

Running @skypack/package-check also validates this.
image

This PR adds export map and after these changes everything is healthy:
image

Package.json "module" is legacy and can (and probably should) be removed. You can also concider adding "type": "module" to properly indicate that this lib is ESM, but again, I'll leave this to the maintainer to decide.

This may also fix: #649

@christjt
Copy link
Contributor Author

christjt commented May 9, 2023

@trusktr Anything you want me to change here?

@trusktr
Copy link
Member

trusktr commented May 9, 2023

This is great! I do agree we should change to type:module and remove the old field.

We would probably have to release this as a major bump, along with a release note too I think.

Would love to get rid of CJS, and just have ESM.

@christjt
Copy link
Contributor Author

This is great! I do agree we should change to type:module and remove the old field.

We would probably have to release this as a major bump, along with a release note too I think.

Would love to get rid of CJS, and just have ESM.

That all sounds great, but we we can add the exports field without it being a breaking change, right? And then in the future considering to remove the commonjs output if you want.

@christjt
Copy link
Contributor Author

@trusktr

@trusktr
Copy link
Member

trusktr commented Jun 23, 2023

@christjt Hello! Sorry for taking so long. I was hesitant because I wanted to try to see if this breaks anything. But I think we can just release this as a major bump, in case it breaks someone (not sure) and note the possibility in the release.

For some reason the build is not running. Do the regular tests pass for you? Can you also add the skypack check to the tests?

After this, let's just merge it and I'll bump the major.

@trusktr trusktr mentioned this pull request Jun 23, 2023
@trusktr
Copy link
Member

trusktr commented Jun 23, 2023

Ok, got the darn build fixed. Looks like it was a bug inside GitHub.

@trusktr
Copy link
Member

trusktr commented Jun 23, 2023

Ok, tests passing, gonna just merge and release a major bump!

@trusktr trusktr merged commit 283e66e into tweenjs:main Jun 23, 2023
3 checks passed
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.

CJS entry point can't be loaded in Node.js
2 participants