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

bgoscinski · 2022-09-02T22:04:32Z

Description

Fixes #9967 by normalizing asset URLs to full absolute URLs with origin and comparing them to link.href properties of all links in the document to find duplicates.

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

bgoscinski · 2022-09-07T14:57:44Z

@patak-dev This is ready for review :)

patak-dev · 2022-09-07T15:58:51Z

I don't think we should modify the way paths work when the base is not relative. You can think about it as an optimization, and there may already be plugins that expect a certain pattern for Vite preload paths. The bug only appears for relative base, so let's fix it for it.

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

bgoscinski · 2022-09-07T19:21:42Z

@patak-dev ready for next round :)

added back relativePreloadUrls and different code for relative and absolute base
added comments clarifying the logic of isPreloaded

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

bgoscinski · 2022-09-07T20:07:20Z

@Tal500 @patak-dev I added test from this PR on top of Tal500's work here https://github.com/bgoscinski/vite/tree/preload-on-system-format-with-9967-test. Unfortunately the test fails :(

Tal500 · 2022-09-07T20:21:25Z

@Tal500 @patak-dev I added test from this PR on top of Tal500's work here https://github.com/bgoscinski/vite/tree/preload-on-system-format-with-9967-test. Unfortunately the test fails :(

Hi! As I said in my comment on you code review, I know that my code isn't perfect (intentionally, because of failing CI) on avoiding duplicated CSS.
The purpose of my PR is different - the partial CSS duplication is only a "side business".
The reason my code isn't fully avoid duplication, is because not both sides of the path equality test are always absolute, and I "cheated" there to avoid failing CI. (Hope you'd find why the CI are failing, assuming some tests are wrong)

The review I sent you was not related to the fact I fail to avoid some duplication.
I was just saying that you can take the idea of instead passing through all the link in the page, per dep, is just do CSS query that takes the relevant links that ends with the same file name, when you trim the parent dir from the URL.
Then, after you have a small set of candidates(e.g. with the hashes it will almost always be exactly one candidate), then check the absolute paths of both sides of the path equation. This just saves so much time for performance, and it's not a too complicated idea.

bgoscinski · 2022-09-07T20:34:58Z

@Tal500 Ahhhh I see! I assumed that your PR is a "superset" of mine and it fixes the bug. That's why I ported my test to your PR :)

In this case I'll borrow the trick with the CSS selector :)

Tal500

Great!

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

bgoscinski · 2022-09-12T16:45:04Z

@patak-dev ready for next pass :) I don't know why CI failed on MacOS but I think this is some kind of a flakiness issue.

Changes summary:

When relativePreloadUrls is false (base isn't relative), then we don't have a bug, no? So we can leave the code as it was before. Or is there a bug here too? IIUC, the bug you reported only happens for relative base.

Addressed. Please see isBaseRelative.

And in this case [base is relative], what we need is to compare new URL(link.href, location.href).href === dep

As I pointed out here and demonstrated here link.href must be absolute (with protocol, host and all the other URL parts). This behavior is also specified here under "If the content attribute is defined to contain a URL". With this in mind new URL(link.href, location.href).href is IMO redundant. If you could please share a scenario that fails without new URL(link.href, location.href).href I'll be happy to add a test for it and implement required changes to make it pass.

I think the test cases may not be correctly testing the issue you reported because CI is passing, or maybe I'm missing something here.

I run my project with Vite built from this branch and bug is gone so I think we're good.

Tal500 · 2022-09-12T17:31:20Z

@patak-dev ready for next pass :) I don't know why CI failed on MacOS but I think this is some kind of a flakiness issue.

Changes summary:

When relativePreloadUrls is false (base isn't relative), then we don't have a bug, no? So we can leave the code as it was before. Or is there a bug here too? IIUC, the bug you reported only happens for relative base.

Addressed. Please see isBaseRelative.

And in this case [base is relative], what we need is to compare new URL(link.href, location.href).href === dep

As I pointed out here and demonstrated here link.href must be absolute (with protocol, host and all the other URL parts). This behavior is also specified here under "If the content attribute is defined to contain a URL". With this in mind new URL(link.href, location.href).href is IMO redundant. If you could please share a scenario that fails without new URL(link.href, location.href).href I'll be happy to add a test for it and implement required changes to make it pass.

I think the test cases may not be correctly testing the issue you reported because CI is passing, or maybe I'm missing something here.

I run my project with Vite built from this branch and bug is gone so I think we're good.

Don't you think that the best, simple thing is to always compare their absolute URLs, no matter what the configuration is?
It's also future safe.

bgoscinski · 2022-09-12T17:56:59Z

@Tal500

Don't you think that the best, simple thing is to always compare their absolute URLs, no matter what the configuration is? It's also future safe.

That's what I initially implemented but @patak-dev suggested to postpone it at least until Vite 4

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

sapphi-red · 2022-09-13T11:33:37Z

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

@@ -75,10 +83,17 @@ function preload(
      seen[dep] = true


This is just an idea but maybe we could simplify the logic if we use new URL(dep, document.baseURI).href. If the SSR markup used an absolute path, I guess the current approach won't work. (it didn't work from before though)

that was what I missed! To supply the base url!
Earlier I used new URL(dep).href that didn't make sense when dep is relative, just understand it now, according to MDN about the constructor signature new URL(url, base):

If url is a relative URL, base is required, and will be used as the base URL. If url is an absolute URL, a given base will be ignored.

This is just an idea but maybe we could simplify the logic if we use new URL(dep, document.baseURI).href. If the SSR markup used an absolute path, I guess the current approach won't work. (it didn't work from before though)

NOOOOOOO! BaseURI isn't supported on IE11!

This is just an idea but maybe we could simplify the logic if we use new URL(dep, document.baseURI).href. If the SSR markup used an absolute path, I guess the current approach won't work. (it didn't work from before though)

NOOOOOOO! BaseURI isn't supported on IE11!

OK, since this PR doesn't introduce legacy support, I think it's fine to use BaseURI.
It seems that a correct place to handle this issue is at my PR #9920, since right now the preload function isn't called for legacy browsers.
Therefore, @bgoscinski, you may ignore the rest of this comment, I'll handle with it after in my PR.

The full solution was introduced in sapper legacy support, on PR sveltejs/sapper#1562.
I'll explain briefly the logic of this sapper PR, how it handle the situation when document.baseURI isn't available:
We could have used the document.URL which is supported by IE11. Sadly, due to the reasons in this stackoverflow answer, if there is some <base> element in the DOM, it changes the base path, ignoring the current URL(demo is included in the stackoverflow answer). Luckily, by the specifications, there could be only one at most such <base> element, so we just need to grab the one we see, and take this to be the baseURI. If no such tag were found, we can simply take document.URL instead.

bgoscinski · 2022-09-14T19:50:46Z

@sapphi-red Ready for next round.

I fixed the bug with <link rel="preload" as="style" href="foo.css"> and modified the tests to make sure this won't regress in the future. Also I didn't quite get what you meant here. Could you please clarify? Also is this something you want to see implemented in this PR?

Aaaand macos build&test failed again... Do you have an idea what might be the cause?

/cc @patak-dev @Tal500

Tal500 · 2022-09-15T07:36:05Z

BTW: Too bad you always squash&force-push the commits to the PR branch. I think it's better for history and the conversation to not do this. Don't worry, when the Vite team merge the PR, it will be squashed, as you can see in the contributing guidelines:

It's OK to have multiple small commits as you work on the PR. GitHub can automatically squash them before merging.

Tal500 · 2022-09-15T07:57:18Z

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

+      const isBaseRelative = !!importerUrl
+
+      // check if the file is already preloaded by SSR markup
+      if (isBaseRelative) {
+        // When isBaseRelative is true then we have `importerUrl` and `dep` is
+        // already converted to an absolute URL by the `assetsURL` function
+        for (let i = links.length - 1; i >= 0; i--) {
+          const link = links[i]
+          // The `links[i].href` is an absolute URL thanks to browser doing the work
+          // for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5
+          if (link.href === dep && (!isCss || link.rel === 'stylesheet')) {
+            return
+          }
+        }
+      } else if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) {
        return
      }


Maybe @sapphi-red suggestion here was to do my initial thought, just in a way that actually fix my issue?
As far as I can tell, it omits the if (isBaseRelative), and the suggestion is to do this:

Suggested change

const isBaseRelative = !!importerUrl

// check if the file is already preloaded by SSR markup

if (isBaseRelative) {

// When isBaseRelative is true then we have `importerUrl` and `dep` is

// already converted to an absolute URL by the `assetsURL` function

for (let i = links.length - 1; i >= 0; i--) {

const link = links[i]

// The `links[i].href` is an absolute URL thanks to browser doing the work

// for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5

if (link.href === dep && (!isCss || link.rel === 'stylesheet')) {

return

}

}

} else if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) {

return

}

const absoluteDep = new URL(dep, document.baseURI).href;

for (let i = links.length - 1; i >= 0; i--) {

const link = links[i]

// The `links[i].href` is an absolute URL thanks to browser doing the work

// for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5

if (link.href === absoluteDep && (!isCss || link.rel === 'stylesheet')) {

return

}

}

bgoscinski · 2022-09-15T08:37:28Z

BTW: Too bad you always squash&force-push the commits to the PR branch. I think it's better for history and the conversation to not do this. Don't worry, when the Vite team merge the PR, it will be squashed, as you can see in the contributing guidelines:

It's OK to have multiple small commits as you work on the PR. GitHub can automatically squash them before merging.

I figured that out some time ago .

sapphi-red · 2022-09-15T11:43:34Z

I fixed the bug with <link rel="preload" as="style" href="foo.css"> and modified the tests to make sure this won't regress in the future.

Thanks!

Also I didn't quite get what you meant here. Could you please clarify?

When an HTML rendered by server used an absolute path for CSS, the CSS will be duplicated.

For example, when document.baseURI is https://example.com/ and the server responded with:

<html>
<head>
  <link rel="stylesheet" href="https://example.com/assets/foo.hash.css">
  <script type="module" src="https://example.com/assets/index.hash.js"></script>
</head>
</html>

__preload will inject <link rel="stylesheet" href="/assets/foo.hash.css">.
But it shouldn't inject it because <link rel="stylesheet" href="https://example.com/assets/foo.hash.css"> already exists.

My idea is basically same to #9970 (comment). (I guess we could use seen variable.)

Also is this something you want to see implemented in this PR?

It's a related to the original bug, but this one isn't a bug caused by this PR. So this could be fixed in a different PR. It's not blocking this PR.

bgoscinski · 2022-09-15T14:03:04Z

@sapphi-red Thanks for clarifying! What else is needed from my side to get this PR ready to merge?

sapphi-red

Thanks! LGTM

The reason is that it's already fixed by vitejs#9970, and we want to make this PR independent.

patak-dev reviewed Sep 7, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

patak-dev reviewed Sep 7, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

patak-dev reviewed Sep 7, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

patak-dev reviewed Sep 7, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

Tal500 reviewed Sep 7, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

Tal500 previously approved these changes Sep 7, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

bgoscinski dismissed Tal500’s stale review via f40b86c September 7, 2022 21:07

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

f4562d7

bgoscinski requested a review from patak-dev September 9, 2022 20:11

Tal500 added a commit to Tal500/vite that referenced this pull request Sep 10, 2022

fix: copy the suggested code from vitejs#9970

6923be3

Tal500 mentioned this pull request Sep 10, 2022

feat: support preloading also on legacy bundle #9920

Open

8 tasks

patak-dev reviewed Sep 12, 2022

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

bgoscinski added 2 commits September 12, 2022 21:24

fix: remove micro optimization

3b09490

fix: precompute absoluteUrls in preload

82dea8e

patak-dev previously approved these changes Sep 12, 2022

View reviewed changes

patak-dev requested a review from sapphi-red September 12, 2022 20:46

sapphi-red reviewed Sep 13, 2022

View reviewed changes

fix: differentiate between rel=preload and rel=stylesheet for CSS files

d199621

bgoscinski dismissed patak-dev’s stale review via d199621 September 13, 2022 18:15

Tal500 reviewed Sep 15, 2022

View reviewed changes

sapphi-red approved these changes Sep 18, 2022

View reviewed changes

sapphi-red requested a review from patak-dev September 18, 2022 03:28

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

patak-dev added this to the 3.2 milestone Sep 18, 2022

patak-dev approved these changes Sep 18, 2022

View reviewed changes

Tal500 approved these changes Sep 18, 2022

View reviewed changes

Tal500 added a commit to Tal500/vite that referenced this pull request Sep 18, 2022

chore: omit the fix for double stylesheets loading

1f05b29

The reason is that it's already fixed by vitejs#9970, and we want to make this PR independent.

patak-dev merged commit 65f97bd into vitejs:main Sep 23, 2022

Tal500 mentioned this pull request Oct 19, 2022

preload does not work and downloaded twice on firefox sveltejs/kit#1042

Closed

sapphi-red mentioned this pull request Oct 26, 2022

Load the index style twice #10641

Closed

7 tasks

benmccann mentioned this pull request Oct 26, 2022

Upgrade to Vite 3.2 sveltejs/kit#7400

Merged

hershelh mentioned this pull request Nov 1, 2022

fix: support dynamic path with alias in new URL() (fix #10597) #10743

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bgoscinski commented Sep 2, 2022

bgoscinski commented Sep 7, 2022

patak-dev commented Sep 7, 2022

bgoscinski commented Sep 7, 2022

bgoscinski commented Sep 7, 2022

Tal500 commented Sep 7, 2022 •

edited

Loading

bgoscinski commented Sep 7, 2022

Tal500 left a comment

bgoscinski commented Sep 12, 2022 •

edited

Loading

Tal500 commented Sep 12, 2022

bgoscinski commented Sep 12, 2022

sapphi-red Sep 13, 2022

Tal500 Sep 15, 2022

Tal500 Sep 15, 2022

Tal500 Sep 15, 2022

bgoscinski commented Sep 14, 2022

Tal500 commented Sep 15, 2022

Tal500 Sep 15, 2022

bgoscinski commented Sep 15, 2022

sapphi-red commented Sep 15, 2022

bgoscinski commented Sep 15, 2022

sapphi-red left a comment

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

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

Conversation

bgoscinski commented Sep 2, 2022

Description

Additional context

What is the purpose of this pull request?

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

bgoscinski commented Sep 7, 2022

patak-dev commented Sep 7, 2022

bgoscinski commented Sep 7, 2022

bgoscinski commented Sep 7, 2022

Tal500 commented Sep 7, 2022 • edited Loading

bgoscinski commented Sep 7, 2022

Tal500 left a comment

Choose a reason for hiding this comment

bgoscinski commented Sep 12, 2022 • edited Loading

Tal500 commented Sep 12, 2022

bgoscinski commented Sep 12, 2022

sapphi-red Sep 13, 2022

Choose a reason for hiding this comment

Tal500 Sep 15, 2022

Choose a reason for hiding this comment

Tal500 Sep 15, 2022

Choose a reason for hiding this comment

Tal500 Sep 15, 2022

Choose a reason for hiding this comment

bgoscinski commented Sep 14, 2022

Tal500 commented Sep 15, 2022

Tal500 Sep 15, 2022

Choose a reason for hiding this comment

bgoscinski commented Sep 15, 2022

sapphi-red commented Sep 15, 2022

bgoscinski commented Sep 15, 2022

sapphi-red left a comment

Choose a reason for hiding this comment

Tal500 commented Sep 7, 2022 •

edited

Loading

bgoscinski commented Sep 12, 2022 •

edited

Loading