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: add node.module export condition #4798

Merged
merged 1 commit into from
Nov 28, 2023
Merged

fix: add node.module export condition #4798

merged 1 commit into from
Nov 28, 2023

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Aug 7, 2023

Description

The ESM saga never ends 💃
This time there's trouble in build tools that understand the node and import conditions but don't support the CJS re-export pattern we use for node to protect against the "Dual Package Hazard" among other things.
How do we solve it?
By adding a condition that bundlers understand, but node ignores: node.module.

What to review

If it builds it works, if anything the bundlesize on a lot of systems will be smaller if they previously used node.import as the ESM to CJS detour adds some fluff. Now they'll cut the fluff and be able to stay in ESM land.

Notes for release

  • fix(esm): add node.module conditions, improves compatibility with Webpack, Astro, Svelte and Vue

@vercel
Copy link

vercel bot commented Aug 7, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Nov 28, 2023 0:57am
studio-workshop 🔄 Building (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 0:57am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 0:57am

@stipsan stipsan force-pushed the add-node-module-condition branch 2 times, most recently from 3e6a34a to 9e32e16 Compare August 7, 2023 13:50
@stipsan
Copy link
Member Author

stipsan commented Aug 8, 2023 •
edited
Loading

Hold off on merging this, we just had an incident come in with next-sanity as the latest version of Next.js has a too agressive minifier that affects styled-components.
Using node.import and the CJS wrapper pattern is somehow protecting us against this bug and we should wait with adopting node.module here until it's fixed.

rexxars
rexxars approved these changes Aug 8, 2023 •
edited
Loading
View reviewed changes
@rexxars rexxars self-requested a review August 8, 2023 14:27
@rexxars rexxars marked this pull request as draft August 8, 2023 14:28
@rexxars
Copy link
Member

rexxars commented Aug 8, 2023

Converted to draft until the next issues are resolved.

@stipsan
Copy link
Member Author

stipsan commented Aug 14, 2023

A fix has landed in a recent canary: vercel/next.js#53831

@rexxars
Copy link
Member

rexxars commented Aug 17, 2023

A fix has landed in a recent canary: vercel/next.js#53831

Does this mean this PR is ready for review now, or that it is no longer necessary?

@stipsan
Copy link
Member Author

stipsan commented Aug 17, 2023

A fix has landed in a recent canary: vercel/next.js#53831

Does this mean this PR is ready for review now, or that it is no longer necessary?

It means we're ready for review :) I'll rebase and fix the conflicts 🙌

@stipsan stipsan force-pushed the add-node-module-condition branch from 9e32e16 to 73522bc Compare August 17, 2023 16:23
@socket-security
Copy link

socket-security bot commented Aug 17, 2023 •
edited
Loading

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

@stipsan stipsan marked this pull request as ready for review August 17, 2023 16:41
@stipsan stipsan force-pushed the add-node-module-condition branch from 73522bc to 9aa331e Compare August 22, 2023 15:05
@stipsan stipsan force-pushed the add-node-module-condition branch 2 times, most recently from 6f73e65 to 2df6cb4 Compare August 22, 2023 15:17
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2023 •
edited
Loading

Component Testing Report Updated Nov 28, 2023 12:59 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 16s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 3s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 9s 3 0 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 7s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 17s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 8s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 40s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 6s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 3s 3 0 0

@stipsan stipsan requested a review from a team August 22, 2023 19:38
@stipsan stipsan mentioned this pull request Aug 23, 2023
Open
@bjoerge bjoerge force-pushed the next branch 2 times, most recently from e7bf4c3 to 4151f2e Compare October 9, 2023 11:02
@bjoerge
Copy link
Member

bjoerge commented Oct 16, 2023 •
edited
Loading

@stipsan is this still relevant? Anything else needs doing here?

@stipsan
Copy link
Member Author

stipsan commented Oct 16, 2023

@stipsan is this still relevant? Anything else needs doing here?

We're seeing signal that it is increasingly relevant in embedded studios for plugins. We recently needed to add it to @sanity/assist for example to fix a build crash.
We haven't seen similar signal for sanity itself at this time, and so it's just a minor enhancement.
At the same time we've seen this pattern cause problems in some build setups, [such as the maintainers of nuxt in @sanity/client](https://github.com/sanity-io/client/pull/318).

Because of the benefit being so small, and there being a risk that it might throw a wrench in someone's build, I'm not sure we should merge this 🤔

@bjoerge bjoerge removed the request for review from a team October 16, 2023 15:29
@bjoerge
Copy link
Member

bjoerge commented Oct 16, 2023

Thanks for clarifying. Let's leave it open for now and revisit later. I've unassigned the dx team as reviwewers, feel free to re-assign if/when you think we should pick it up again.

skogsmaskin
skogsmaskin approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@skogsmaskin skogsmaskin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as code owner (not really relevant here, DX should hold this)

@stipsan stipsan force-pushed the add-node-module-condition branch 2 times, most recently from b03d130 to 994c8e7 Compare November 28, 2023 09:57
@github-actions GitHub Actions
Copy link
Contributor

No changes to documentation

@stipsan stipsan force-pushed the add-node-module-condition branch from 994c8e7 to 066009b Compare November 28, 2023 12:53
@stipsan stipsan requested a review from a team November 28, 2023 12:53
@bjoerge bjoerge added this pull request to the merge queue Nov 28, 2023
Merged via the queue into next with commit 50ee66e Nov 28, 2023
35 checks passed
@bjoerge bjoerge deleted the add-node-module-condition branch November 28, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skogsmaskin skogsmaskin skogsmaskin approved these changes

@rexxars rexxars Awaiting requested review from rexxars

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stipsan @rexxars @bjoerge @skogsmaskin