-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(turbopack): Update app-renderer #4102
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
🟢 CI successful 🟢Thanks |
Benchmark for 6db060b
Click to view full benchmark
|
return __entry_css_files__; | ||
} | ||
return new Proxy({}, proxyMethodsForModule(name as string, css)); | ||
const [file, name] = (key as string).split("#"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file could potentially contain #
and the name
could also potentially contain #
. This is pretty unsafe to join with #
.
Can't we encode all information into the JSON instead of joining with #
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's a Next.js detail and what's a Turbopack detail. Is the name
really supposed to be JSON, or is that a hack that we did? If we instead did an rsplit here, would that fix our hack without changing Next's behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSON.parse
is a hack inside Turbopack. That #
was from React's upstream change: facebook/react#26300 and we had to encode it that way.
name
can never contain #
because it's the module's export name (export const foo
) or *
. But not sure if a filepath can contain the #
symbol or not, I think it's unlikely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The react change changes from filename
and name
as separate entities to a single id
. next.js creates the file#name
encoding here: https://github.com/vercel/next.js/blob/7e933a094c3f7a17ae085396633ad6525ccfb590/packages/next/src/build/webpack/loaders/next-flight-loader/module-proxy.ts#L168
We could also choose a different encoding, e. g. JSON.
folders can contain #
, unlikely but some people do that /home/blah/#repos
which already led to some trouble because we had some special behavior for fragments in webpack.
even export names will soon be able to support any string. There is a proposal for that.
But anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to use an rsplit.
Reverts #46861. This requires vercel/turborepo#4102 to be released and bindings to be updated first. ~Also depends on #46896 to be merged first, and conflicts to be resolved.~
Benchmark for 87275b3
Click to view full benchmark
|
Benchmark for 5c47b33
Click to view full benchmark
|
### Description This PR mirrors some related manifest changes in Next.js: #46881. ### Testing Instructions <!-- Give a quick description of steps to test your changes. --> --------- Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
### Description This PR mirrors some related manifest changes in Next.js: #46881. ### Testing Instructions <!-- Give a quick description of steps to test your changes. --> --------- Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
### Description This PR mirrors some related manifest changes in Next.js: #46881. ### Testing Instructions <!-- Give a quick description of steps to test your changes. --> --------- Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
### Description This PR mirrors some related manifest changes in Next.js: #46881. ### Testing Instructions <!-- Give a quick description of steps to test your changes. --> --------- Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
### Description This PR mirrors some related manifest changes in Next.js: #46881. ### Testing Instructions <!-- Give a quick description of steps to test your changes. --> --------- Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Description
This PR mirrors some related manifest changes in Next.js: vercel/next.js#46881.
Testing Instructions