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: emit ESM with .mjs #1038

Closed
wants to merge 1 commit into from
Closed

fix: emit ESM with .mjs #1038

wants to merge 1 commit into from

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Dec 30, 2022

@vercel
Copy link

vercel bot commented Dec 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sanity-ui-workshop ✅ Ready (Inspect) Visit Preview Dec 30, 2022 at 10:44PM (UTC)

@mariuslundgard
Copy link
Member

mariuslundgard commented Jan 1, 2023

Unfortunately this project is not ready for .mjs extensions (or "type": "module") because of the peer dependency on styled-components.

@stipsan
Copy link
Member Author

stipsan commented Jan 1, 2023

@mariuslundgard this PR does not add type: module, it adds .mjs to fix said regression.
Not merging this PR means it's impossible to use Studio v3 in Next.js apps without locking @sanity/ui to 1.0.4 or older 😅

@stipsan
Copy link
Member Author

stipsan commented Jan 1, 2023

@mariuslundgard
Copy link
Member

Yes, we'll need to revert "type": "commonjs".

@mariuslundgard
Copy link
Member

mariuslundgard commented Jan 1, 2023

Fwiw, I tested with Next.js before releasing and it worked fine for me. Will do some more testing to evaluate how I got it wrong.

@stipsan
Copy link
Member Author

stipsan commented Jan 1, 2023

Yeah it's happened to me multiple times as well, especially while adding ESM to next-sanity. The only surefire way to test that I've found is to do a prerelease on npm and test next dev and next build with that.
Anything involving symlinks or messing around in node_modules can't be trusted 😢

@mariuslundgard
Copy link
Member

This means Next.js does not interpret packages the same way Node.js does. We should point this out to the Next.js team, since it potentially hinders Node ESM adoption.

@stipsan
Copy link
Member Author

stipsan commented Jan 1, 2023

I'm not sure if that's the case. Adding type: commonjs means only .mjs files may use ESM syntax. The errors we're seeing is correct in that sense.
The problematic Next.js behavior I'm referring to is that it'll sometimes transpile symlinked packages and create a false positive that way, when it should rather consistently fail when defining pkg.type to either commonjs or module.

@mariuslundgard
Copy link
Member

I'm adding a change to @sanity/pkg-utils to make the revert possible: sanity-io/pkg-utils#37

@mariuslundgard
Copy link
Member

mariuslundgard commented Jan 2, 2023

I'm not sure if that's the case. Adding type: commonjs means only .mjs files may use ESM syntax. The errors we're seeing is correct in that sense.

This is from the Node spec, @stipsan:

If the nearest parent package.json lacks a "type" field, or contains "type": "commonjs", .js files are treated as CommonJS.

@mariuslundgard
Copy link
Member

Closing this PR in favor of aa51484

@mariuslundgard mariuslundgard deleted the fix-emit-ESM-with-mjs branch December 7, 2023 14:05
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.

None yet

2 participants