Relative `base` option + dynamic import leads to styles injected two times in production build #9967

bgoscinski · 2022-09-02T13:22:19Z

Describe the bug

When base is set to relative (empty string or './') then in prod build when dynamically importing a module that has some CSS dependencies these dependencies will be appended second time to <head> leading to wrong styles. In vite serve scenario this seem to work fine.

I managed to find possible root cause:

When base is absolute then the generated assetsUrl returns absolute URLs without origin part which match the ones vite embeds into index.html 👍:

// ./assets/index.888728d5.js
const assetsURL = function(dep) {
  return "/" + dep;
};

// index.html
<script type="module" crossorigin src="/assets/index.888728d5.js"></script>
<link rel="stylesheet" href="/assets/index.6744de28.css">

However when base is relative then assetsUrl returns absolute URLs with origin part and vite embeds relative URLs into index.html 🐛:

// ./assets/index.46a59a36.js
const assetsURL = function(dep, importerUrl) {
  // This returns for example 'https://some.origin/foo/bar/assets/index.6744de28.css'
  return new URL(dep, importerUrl).href;
};

// index.html
<script type="module" crossorigin src="./assets/index.46a59a36.js"></script>
<link rel="stylesheet" href="./assets/index.6744de28.css">

This mismatch in relative base case causes __vitePreload to think that given asset was not yet loaded because dep is an absolute URL with origin and index.html has relative URL to the file.

// __vitePreload function
if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) {
  return; // <=========== This early return is not reached
}

To reproduce this bug

Clone https://github.com/bgoscinski/vite-relative-base-plus-dynamic-import-repro
npm i && npx vite build && npx vite preview
Open the preview. Background should be green
Click the button. Background should remain green but it changes to red 🐛. This happens because there is a second linkto the same CSS file at the bottom of <head>. This second one "wins" the style cascade now :(
Kill the preview
Remove base option from vite.config.js
npx vite build && npx vite preview
Open the preview. Background should be green
Click the button. Background remains green 👍

Reproduction

https://github.com/bgoscinski/vite-relative-base-plus-dynamic-import-repro

System Info

$ npx envinfo --system --npmPackages '{vite,@vitejs/*}' --binaries --browsers

  System:
    OS: Windows 10 10.0.19044
    CPU: (8) x64 Intel(R) Core(TM) i5-9300H CPU @ 2.40GHz
    Memory: 6.84 GB / 15.86 GB
  Binaries:
    Node: 16.13.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.19.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 104.0.5112.102
    Edge: Spartan (44.19041.1266.0), Chromium (104.0.1293.70)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    vite: ^3.0.9 => 3.0.9

Used Package Manager

npm

Logs

No response

Validations

Follow our Code of Conduct
Read the Contributing Guidelines.
Read the docs.
Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
The provided reproduction is a minimal reproducible example of the bug.

The text was updated successfully, but these errors were encountered:

patak-dev · 2022-09-02T13:52:41Z

@bgoscinski great findings! Let us know if you would like to work on a PR to fix this 👍🏼

bgoscinski · 2022-09-02T14:37:30Z

Sure, I can get my hands dirty, but I'd need some guidance about how should I fix it. I can see few ways it can be fixed:

Making assetsURL return a URL relative to index.html when base is relative. I'm not sure if it's so simple to get full index.html URL though.
Somehow populate seen object with absolute URLs of styles that vite embeds into index.html as soon as the app loads (before any dynamic import could run)
Replace document.querySelector(`link[href="${dep}"]${cssSelector}`) check with something like document.querySelectorAll('link').some(link => link.href === dep && (!isCss || link.rel === 'stylesheet')). This option takes advantage of link.href property being always an absolute URL with origin (while the attribute can be relative)

patak-dev · 2022-09-02T15:04:45Z

I think something along the lines of 3 is the best, as even with two, people may add a link to a CSS by hand that we will miss with 2. For the moment, you could use dep before being modified here

vite/packages/vite/src/node/plugins/importAnalysisBuild.ts

Line 71 in 4158b98

dep = assetsURL(dep, importerUrl)

But after #9938, this function may already get a full URL. Maybe we could find a way to compare full URL with relative ones. It isn't clear at this point what is best (more after #9938)

bgoscinski · 2022-09-02T15:10:58Z

Yeah, I think it's safest to always (with both relative and absolute base option) normalize dep to full absolute URL (like https://some.origin/foo/bar/assets/index.6744de28.css) and then compare it to each link.href property which is also a full absolute URL.

On it.

Tal500 · 2022-09-06T10:58:11Z

Hi! I was noticing this on my work on PR #9920, and I did fixed it there also, by this code in the preload function:

vite/packages/vite/src/node/plugins/importAnalysisBuild.ts

Lines 70 to 92 in 8f68cdf

    
           // @ts-ignore 
        
           dep = assetsURL(dep, importerUrl) 
        
           // @ts-ignore 
        
           if (dep in seen) return 
        
           // @ts-ignore 
        
           seen[dep] = true 
        
           const seperatorIdx = dep.lastIndexOf('/') 
        
           const shortDep = seperatorIdx >= 0 ? dep.slice(seperatorIdx + 1) : dep 
        
           const isCss = dep.endsWith('.css') 
        
           const cssSelector = isCss ? '[rel="stylesheet"]' : '' 
        
           // @ts-ignore check if the file is already preloaded by SSR markup 
        
           const possibleLinks = document.querySelectorAll<HTMLLinkElement>( 
        
             `link[href$="${shortDep}"]${cssSelector}` 
        
           ) 
        
           for (let i = 0; i < possibleLinks.length; ++i) { 
        
             const currentPath = possibleLinks[i].href 
        
             if ( 
        
               currentPath === dep || 
        
               new URL(currentPath, importerUrl).href === dep 
        
             ) { 
        
               return 
        
             } 
        
           }

What do you think?

bgoscinski added the pending triage label Sep 2, 2022

patak-dev added bug feat: css p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Sep 2, 2022

bgoscinski mentioned this issue Sep 2, 2022

fix: don't duplicate styles with dynamic import (fix #9967) #9970

Merged

9 tasks

Tal500 mentioned this issue Sep 6, 2022

feat: support preloading also on legacy bundle #9920

Open

8 tasks

patak-dev closed this as completed in #9970 Sep 23, 2022

patak-dev pushed a commit that referenced this issue Sep 23, 2022

fix: don't duplicate styles with dynamic import (fix #9967) (#9970)

65f97bd

github-actions bot locked and limited conversation to collaborators Oct 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relative `base` option + dynamic import leads to styles injected two times in production build #9967

Relative `base` option + dynamic import leads to styles injected two times in production build #9967

bgoscinski commented Sep 2, 2022 •

edited

Loading

patak-dev commented Sep 2, 2022

bgoscinski commented Sep 2, 2022 •

edited

Loading

patak-dev commented Sep 2, 2022

bgoscinski commented Sep 2, 2022

Tal500 commented Sep 6, 2022

Relative base option + dynamic import leads to styles injected two times in production build #9967

Relative base option + dynamic import leads to styles injected two times in production build #9967

Comments

bgoscinski commented Sep 2, 2022 • edited Loading

Describe the bug

I managed to find possible root cause:

To reproduce this bug

Reproduction

System Info

Used Package Manager

Logs

Validations

patak-dev commented Sep 2, 2022

bgoscinski commented Sep 2, 2022 • edited Loading

patak-dev commented Sep 2, 2022

bgoscinski commented Sep 2, 2022

Tal500 commented Sep 6, 2022

Relative `base` option + dynamic import leads to styles injected two times in production build #9967

Relative `base` option + dynamic import leads to styles injected two times in production build #9967

bgoscinski commented Sep 2, 2022 •

edited

Loading

bgoscinski commented Sep 2, 2022 •

edited

Loading