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

Emit polyfill-nomodule.js into the build manifest polyfillFiles #65223

Merged
merged 4 commits into from
May 3, 2024

Conversation

bgw
Copy link
Member

@bgw bgw commented May 1, 2024

Why?

polyfill-nomodule.js is a pre-built file containing polyfills for older browsers (gated by the <script> tag nomodule attribute).

How?

  • 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.

Test Plan

HEADLESS=true pnpm testonly-dev test/e2e/app-dir/app/index.test.ts -t 'should serve polyfills for browsers that do not support modules'
HEADLESS=true pnpm testonly-dev-turbo test/e2e/app-dir/app/index.test.ts -t 'should serve polyfills for browsers that do not support modules'

Build a project with next dev --turbo and inspect:

Screenshot 2024-05-01 at 10.40.39 PM.png
Screenshot 2024-05-01 at 10.40.20 PM.png

Verify that the polyfill file exists and resolves in the browser.

Closes PACK-2993

@ijjk ijjk added created-by: Turbopack team PRs by the Turbopack team. Turbopack Related to Turbopack with Next.js. type: next labels May 1, 2024
Copy link
Member Author

bgw commented May 1, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bgw and the rest of your teammates on Graphite Graphite

@bgw bgw marked this pull request as ready for review May 1, 2024 03:14
@bgw bgw marked this pull request as draft May 1, 2024 03:16
@ijjk

This comment was marked as outdated.

@ijjk

This comment was marked as resolved.

@ijjk

This comment was marked as outdated.

Comment on lines 910 to 911
let polyfill_output_asset =
VirtualOutputAsset::new(polyfill_output_path, polyfill_module.content());
Copy link
Member

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:

Suggested change
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.

Copy link
Member Author

@bgw bgw May 2, 2024

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

@ijjk ijjk added the tests label May 2, 2024
@bgw bgw marked this pull request as ready for review May 2, 2024 22:14
@bgw bgw requested a review from wbinnssmith May 2, 2024 22:17
bgw added a commit to vercel/turborepo that referenced this pull request May 2, 2024
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`.
bgw added a commit that referenced this pull request May 2, 2024
Use updated API suggested by sokra in review on #65223:

#65223 (comment)
bgw added 4 commits May 2, 2024 16:47
- `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.
bgw added a commit that referenced this pull request May 2, 2024
Use updated API suggested by sokra in review on #65223:

#65223 (comment)
@bgw bgw merged commit eba364a into canary May 3, 2024
79 of 80 checks passed
@bgw bgw deleted the bgw/polyfill-nomodule branch May 3, 2024 05:15
bgw added a commit to vercel/turborepo that referenced this pull request May 8, 2024
### 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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Turbopack team PRs by the Turbopack team. locked tests Turbopack Related to Turbopack with Next.js. type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants