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

feat(node/build): add crossOrigin configuration for modulePreload links #13136

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

HipsterBrown
Copy link

@HipsterBrown HipsterBrown commented May 9, 2023

Description

Addresses #6648

My team and I are integrating Vite into an app that is currently using Webpack. We ran into an issue while attempting to deploy updated application to the various production-like environments, i.e. staging, demo, etc, that use different asset hosts with strict CORS settings. The module preload behavior used by Vite is encountering issues when preloading the chunks of dynamically imported modules due to the empty crossorigin attribute on the generated <link> tags.

This PR resolves the issue by adding a configuration field to the build.modulePreload object to disable or set the crossorigin attribute with one of the accepted string values.

I've tested this patch in the app locally with success.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented May 9, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ketulm
Copy link

ketulm commented Jun 2, 2023

We are hitting an issue with the modulePreload links getting blocked on Firefox due to the crossorigin being defaulted to anonymous and thus not including the credentials for those requests. This PR seems like it would resolve that issue and allow overriding the default behavior.

Are there any updates on this PR getting in? Or is there any workaround? Either via plugins or overriding the polyfill?

@ketulm
Copy link

ketulm commented Jun 5, 2023

It would also be good to make use of this new option in html.ts so that the newly created link tags use the credentials as well.

Something along the lines of the following, which is what worked for the Firefox issue due to the way polyfill relies on crossorigin to set the credentials:

@@ -23,7 +23,7 @@ import {
   processSrcSet,
   removeLeadingSlash,
 } from '../utils'
-import type { ResolvedConfig } from '../config'
+import type { ModulePreloadOptions, ResolvedConfig } from '../config'
 import { toOutputFilePathInHtml } from '../build'
 import { resolveEnvPrefix } from '../env'
 import {
@@ -625,12 +625,13 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
         chunk: OutputChunk,
         toOutputPath: (filename: string) => string,
         isAsync: boolean,
+        crossorigin: ModulePreloadOptions['crossOrigin'],
       ): HtmlTagDescriptor => ({
         tag: 'script',
         attrs: {
           ...(isAsync ? { async: true } : {}),
           type: 'module',
-          crossorigin: true,
+          crossorigin: crossorigin ?? true,
           src: toOutputPath(chunk.fileName),
         },
       })
@@ -638,11 +639,12 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
       const toPreloadTag = (
         filename: string,
         toOutputPath: (filename: string) => string,
+        crossorigin: ModulePreloadOptions['crossOrigin'],
       ): HtmlTagDescriptor => ({
         tag: 'link',
         attrs: {
           rel: 'modulepreload',
-          crossorigin: true,
+          crossorigin: crossorigin ?? true,
           href: toOutputPath(filename),
         },
       })
@@ -736,12 +738,16 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
           // when inlined, discard entry chunk and inject <script> for everything in post-order
           const imports = getImportedChunks(chunk)
           let assetTags: HtmlTagDescriptor[]
+          const { modulePreload } = config.build
+          const { crossOrigin } = modulePreload as ModulePreloadOptions
           if (canInlineEntry) {
             assetTags = imports.map((chunk) =>
-              toScriptTag(chunk, toOutputAssetFilePath, isAsync),
+              toScriptTag(chunk, toOutputAssetFilePath, isAsync, crossOrigin),
             )
           } else {
-            assetTags = [toScriptTag(chunk, toOutputAssetFilePath, isAsync)]
+            assetTags = [
+              toScriptTag(chunk, toOutputAssetFilePath, isAsync, crossOrigin),
+            ]
             const { modulePreload } = config.build
             if (modulePreload !== false) {
               const resolveDependencies =
@@ -756,7 +762,7 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
                 : importsFileNames
               assetTags.push(
                 ...resolvedDeps.map((i) =>
-                  toPreloadTag(i, toOutputAssetFilePath),
+                  toPreloadTag(i, toOutputAssetFilePath, crossOrigin),
                 ),
               )
             }

@patak-dev
Copy link
Member

I think a global option makes more sense here. Do you see a reason to use a different crossOrigin for regular script tags and preload tags? Maybe build.html.crossOrigin. We don't have other similar options though. I'll add this PR to the team's board so we get to discuss it in a future team meeting.

@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Jun 5, 2023
@ketulm
Copy link

ketulm commented Jun 5, 2023

For our use case, the entire app requires credentials to load (all assets including the bundle). So, for our immediate use case, there wouldn't be a need for a separate option for regular tags and preload tags.

It'd be good to have the preloading work the same way as Chromium given that there are no issues there with preloading the bundle.

@HipsterBrown
Copy link
Author

@patak-dev

I think a global option makes more sense here. Do you see a reason to use a different crossOrigin for regular script tags and preload tags?

Thanks for taking a look at this PR! A global option like build.html.crossOrigin does make more sense when including the additions mentioned by @ketulm. I opted for extending the existing config option since the original goal for this PR was to affect the module preload scripts only. If the team agrees on making this a global option for the html output, I can update the PR to address that feedback.

Cheers

Copy link

@yumanman88 yumanman88 left a comment

Choose a reason for hiding this comment

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

great

@sapphi-red
Copy link
Member

Do you see a reason to use a different crossOrigin for regular script tags and preload tags?

The static/dynamic imports uses the same crossorigin value (tc39/proposal-dynamic-import#60, spec). This means that a single crossorigin value is used for the whole module graph. So a preload tag should always use the same crossorigin value with the script tag. Otherwise the preload will be meaningless.

Given that CSP nonce also needs to add a attribute to <link rel="modulepreload"> (#14653), that might be also put in build.html. (Although, for nonce, it can be obtained from the HTML)


Also I was thinking about whether we can get the crossOrigin value automatically. But I didn't find a way to do that.
I thought it's possible to get it by checking the script tag. But for the following example, I guess mod3.js will be fetched with credentials sometimes and without it sometimes.

<script type="module" src="mod1.js" crossorigin /> <!-- mod1.js imports mod3.js -->
<script type="module" src="mod2.js" crossorigin="use-credentials" /> <!-- mod2.js imports mod3.js -->

Even if we ignore this case, there's still a problem. For the following example, we need to detect which script tag is the one importing mod3.js.

<script type="module" src="mod1.js" crossorigin /> <!-- mod1.js doesn't import mod3.js -->
<script type="module" src="mod2.js" crossorigin="use-credentials" /> <!-- mod2.js imports mod3.js -->

If Vite is generating the HTML, we can inject a marker (e.g. add vite-bundle-id attribute to script tag). But that requires the frameworks/backend integrations to inject that.

@ling317
Copy link

ling317 commented Jun 6, 2024

什么时候才能增加该功能

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: Later
Development

Successfully merging this pull request may close these issues.

6 participants