-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Emit polyfill-nomodule.js into the build manifest polyfillFiles #65223
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
let polyfill_output_asset = | ||
VirtualOutputAsset::new(polyfill_output_path, polyfill_module.content()); |
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.
It doesn't really matter here, but you could change it to:
let polyfill_output_asset = | |
VirtualOutputAsset::new(polyfill_output_path, polyfill_module.content()); | |
let polyfill_output_asset = | |
RawOutput::new(polyfill_output_path, polyfill_module); |
If you give RawOutput
an additional path
argument, which it reports as ident()
AssetIdent::from_path(path)
. That would allow the RawOutput
to "serve" a Source
under a specific path.
The motivation behind that:
Eventually we want to have lazy compilation. That would mean instead of emitting all OutputAssets directly to disk we would directly serve them on request. That would mean Asset::content()
on OutputAssets is only called when the file is actually requested. So we want to delay as much work as possible to this point. e. g. Chunks only generate the content in the content()
method, so code generation is skipped until this point.
By having RawOutput::new(polyfill_output_path, polyfill_module)
the polyfill_module.content()
method would only be called when the file is requested. So that would cause the polyfill file not to be read until it's actually requested from the browser.
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.
Because this requires changes to turbopack_core
, I'm going to leave it as VirtualOutputAsset
in this PR, but I'll make a separate PR for the turbo repo to add a path to RawOutput
, and a follow-up to switch this to RawOutput
.
Edit: vercel/turborepo#8075 and #65300
Updated API suggested by @sokra in review on #65223: vercel/next.js#65223 (comment) Without this change, I don't think this API is useful. This makes the API very similar to `VirtualOutputAsset`.
Use updated API suggested by sokra in review on #65223: #65223 (comment)
- `polyfill-nomodule.js` is a pre-built file containing polyfills for older browsers (gated by the `<script>` tag `nomodule` attribute). - The turbopack server needs to emit a raw OutputAsset for this file, so that it is copied into the output chunks directory. - That file needs to be passed into `polyfillFiles`, and preserved when we're merging manifests inside of the development server.
Use updated API suggested by sokra in review on #65223: #65223 (comment)
### Description Update `RawOutput` API Updated API suggested by @sokra in review on #65223: vercel/next.js#65223 (comment) Without this change, I don't think this API is useful. This makes the API very similar to `VirtualOutputAsset`. ### Testing Instructions Tested with vercel/next.js#65300 Closes PACK-3034
Why?
polyfill-nomodule.js
is a pre-built file containing polyfills for older browsers (gated by the<script>
tagnomodule
attribute).How?
polyfillFiles
, and preserved when we're merging manifests inside of the development server.Test Plan
Build a project with
next dev --turbo
and inspect:Verify that the polyfill file exists and resolves in the browser.
Closes PACK-2993