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

Turbopack: EcmascriptChunkItemContent: add react refresh parameter based on module #8133

Merged
merged 7 commits into from
May 15, 2024

Conversation

wbinnssmith
Copy link
Member

Rather than reading it from the chunking context, which applies to all modules for a given chunking target, read it from the EcmascriptOptions for the module. This ensures that we only make react refresh values available to user code transformed with it.

This also:

  • Removes EcmascriptChunkingContext, as it’s no longer necessary
  • Makes EcmascriptOptions a Vc in many more cases

Test Plan: Integration tests

…based on module

Rather than reading it from the chunking context, which applies to all modules for a given chunking target, read it from the `EcmascriptOptions` for the module. This ensures that we only make react refresh values available to user code transformed with it.

This also:

- Removes `EcmascriptChunkingContext`, as it’s no longer necessary
- Makes `EcmascriptOptions` a Vc in many more cases

Test Plan: Integration tests
Copy link

vercel bot commented May 13, 2024

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

Name Status Preview Comments Updated (UTC)
examples-gatsby-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback May 14, 2024 6:58am
examples-kitchensink-blog 🔄 Building (Inspect) Visit Preview 💬 Add feedback May 14, 2024 6:58am
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2024 6:58am
examples-svelte-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback May 14, 2024 6:58am
rust-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2024 6:58am
5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 6:58am
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 6:58am
examples-native-web ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 6:58am
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 6:58am
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview May 14, 2024 6:58am

Copy link
Contributor

github-actions bot commented May 13, 2024

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

Copy link
Contributor

github-actions bot commented May 13, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

✅ This change can build next-swc

Copy link
Member

@bgw bgw left a comment

Choose a reason for hiding this comment

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

Nice, it seems like the removal of EcmascriptChunkingContext simplifies things quite a bit, especially in the cases that needed downcasting.

@wbinnssmith wbinnssmith merged commit 27223f1 into main May 15, 2024
48 of 50 checks passed
@wbinnssmith wbinnssmith deleted the wbinnssmith/module-refresh branch May 15, 2024 00:08
wbinnssmith added a commit to vercel/next.js that referenced this pull request May 15, 2024
panteliselef pushed a commit to panteliselef/next.js that referenced this pull request May 20, 2024
@@ -162,7 +162,7 @@ async fn into_ecmascript_module_asset(
analyze_types: false,
}),
this.transforms,
Value::new(Default::default()),
EcmascriptOptions::default().cell(),
Copy link
Member

Choose a reason for hiding this comment

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

This does kinda need refresh

ForsakenHarmony added a commit that referenced this pull request Jun 13, 2024
### Description

Seems like this was missed in #8133

### Testing Instructions

Closes PACK-3115
Neosoulink pushed a commit to Neosoulink/turbo that referenced this pull request Jun 14, 2024
…based on module (vercel#8133)

Rather than reading it from the chunking context, which applies to all
modules for a given chunking target, read it from the
`EcmascriptOptions` for the module. This ensures that we only make react
refresh values available to user code transformed with it.

This also:

- Removes `EcmascriptChunkingContext`, as it’s no longer necessary
- Makes `EcmascriptOptions` a Vc in many more cases

Test Plan: Integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants