fix: call createFileOnlyEntry() only for CSS deps, fix #4150

[paraboul]

Description

Tailwind (JIT) registers all purgeable files as dependencies of the generated css.
This lead to every SFC/html being added to moduleGraph with createFileOnlyEntry().

If some cases, createFileOnlyEntry() is called before ensureEntryFromUrl() for the same file but with a different URL (because createFileOnlyEntry() only add modules entries with absolute path using FS_PREFIX), ultimately leading to the same module having a duplicate entry but with different importers (preventing HMR from properly propagating updates).

This PR tries to address this problem by making sure we're calling ensureEntryFromUrl() rather than createFileOnlyEntry() for non CSS files

This fix #4150

Additional context

Discussion on the issue #4150

---

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.

[patak-dev]

Thanks for digging into this issue @paraboul, reading your comments has been very helpful to understand the problem. I think the PR is close to where we need it, but it is fragile to call createFileOnlyEntry with something that is not a file-only entry (some of the maps are not going to be correctly updated, like urlToModuleMap).

Check out the comment createFileOnlyEntry

vite/packages/vite/src/node/server/moduleGraph.ts

Line 165 in c45a02f

createFileOnlyEntry(file: string): ModuleNode {

  // some deps, like a css file referenced via @import, don't have its own
  // url because they are inlined into the main css import. But they still
  // need to be represented in the module graph so that they can trigger
  // hmr in the importing css file.

This should only be called for these CSS deps, and not for the others. I think we should apply the same pattern that is already present here

vite/packages/vite/src/node/server/hmr.ts

Line 230 in 09c6c94

if (cssLangRE.test(importer.url) && !currentChain.includes(importer)) {

and only call createFileOnlyEntry here

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

Line 195 in 09c6c94

[...deps].map((file) => moduleGraph.createFileOnlyEntry(file))

if the dep a CSS dep of that CSS module, and for others deps (for example the ones registered by postCSS) it should call ensureEntryFromUrl as usual.

What do you think?

@OneNail your input here would also be appreciated :)

[paraboul]

@patak-js Thanks for the feedback! :)

I'm not sure to understand when createFileOnlyEntry() could be called with something that is not a file?

It's called from this place (and only there) since it has been added by this commit :
b5479b8

The problem here is that createFileOnlyEntry() is called for non-css file that are then registered by ensureEntryFromUrl() with a different file location (FS_PATH+absolute vs relative to the root project).

This PR really just tries to make sure the same module is being unified in one entry if createFileOnlyEntry() is called first.

Or am I missing something?

[patak-dev]

I'm not sure to understand when createFileOnlyEntry() could be called with something that is not a file?

Not with something that is not a file, but it should only be called for file-only entries as in a CSS imported from a CSS by a preprocessor that will end up inlined (and it isn't then a real independent module)

It's called from this place (and only there) since it has been added by this commit :
b5479b8

The problem here is that createFileOnlyEntry() is called for non-css file that are then registered by ensureEntryFromUrl() with a different file location (FS_PATH+absolute vs relative to the root project).

This PR really just tries to make sure the same module is being unified in one entry if createFileOnlyEntry() is called first.

Yes, I understand what you were trying to achieve. The issue is that these deps added by postCSS are not file-only entries and with the current change set they will end up in the module graph without a proper entry in urlToModuleMap for example.

What I mean in my last comment is that we should have something like

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

Line 195 in 09c6c94

[...deps].map((file) => moduleGraph.createFileOnlyEntry(file))

Warning: pseudocode, not tested

          // record deps in the module graph so edits to @import css can trigger
          // main import to hot update. PostCSS can add other deps to a CSS, so 
          // we need to check if this is file-only entry and fallback to a regular
          // module entry for other modules
          const depModules = new Set(
            [...deps].map((file) =>
              cssLangRE.test(file) ?
                moduleGraph.createFileOnlyEntry(file) :
                moduleGraph.ensureEntryFromUrl(file)
          )

I haven't checked the details here, maybe the file needs to be converted to an URL before passing it to ensureEntryFromUrl for example.

[paraboul]

@patak-js Ok I get it.

Thanks!

[y1d7ng]

Thanks for digging into this issue @paraboul, reading your comments has been very helpful to understand the problem. I think the PR is close to where we need it, but it is fragile to call createFileOnlyEntry with something that is not a file-only entry (some of the maps are not going to be correctly updated, like urlToModuleMap).

Check out the comment createFileOnlyEntry

vite/packages/vite/src/node/server/moduleGraph.ts

Line 165 in c45a02f

createFileOnlyEntry(file: string): ModuleNode {

  // some deps, like a css file referenced via @import, don't have its own
  // url because they are inlined into the main css import. But they still
  // need to be represented in the module graph so that they can trigger
  // hmr in the importing css file.

This should only be called for these CSS deps, and not for the others. I think we should apply the same pattern that is already present here

vite/packages/vite/src/node/server/hmr.ts

Line 230 in 09c6c94

if (cssLangRE.test(importer.url) && !currentChain.includes(importer)) {

and only call createFileOnlyEntry here

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

Line 195 in 09c6c94

[...deps].map((file) => moduleGraph.createFileOnlyEntry(file))

if the dep a CSS dep of that CSS module, and for others deps (for example the ones registered by postCSS) it should call ensureEntryFromUrl as usual.
What do you think?

@OneNail your input here would also be appreciated :)

It's very distressing to not be able to reproduce the bug #4150 before. Thank @paraboul for reproducing and solving this bug. It turns out that vue-router andd dynamic import let ensureEntryFromUrl be executed after createFileOnlyEntry. I personally prefer the solution @patak-js you proposed, it looks more robust.

[paraboul]

@patak-js
Here you go. Should be better now!

[paraboul]

@OneNail For reference, it's not specific to vue-router. The problem happens as soon as a CSS plugin registers non CSS files as dependencies of the transformed CSS (which is what Tailwind JIT does). The issue exhibits as soon as a purgeable file doesn't import any other dependency.

[y1d7ng]

@OneNail For reference, it's not specific to vue-router. The problem happens as soon as a CSS plugin registers non CSS files as dependencies of the transformed CSS (which is what Tailwind JIT does). The issue exhibits as soon as a purgeable file doesn't import any other dependency.

Yes, the root cause is what you pointed out, vue-router just allows this bug to be reproduced, which made me very distressed before.

[patak-dev]

@anncwb would you test this PR with your use case #4150 (comment)?

[patak-dev]

@knightly-bot build this

[knightly-bot]

🌒 Knightly build enabled, release every night at 00:00 UTC ^{(skip if no change)}

📦 npm package

[patak-dev]

@jods4 @yassinebridi, @luukdv, @cossssmin, @nmabhinandan the Knightly bot built a version of this PR (link in the previous comment). Would you help us test this using it in your projects? Thanks!

[eugene1g]

@paraboul @patak-js
FWIW, this PR fixes the issue in my project (React + Tailwind JIT) - thank you!

I tried to isolate the problem to add a test but cannot reproduce/recreate it within playground :/ Still, the fix worked for my complex projects so this change is much appreciated.

[eugene1g]

@paraboul I think I isolated this - are you working on a test or should I?

Edit: Nevermind - I had it, then tried to simplify further, and lost it. Whac-A-Mole
.

[paraboul]

I can check early next week. Unless you already have something ready ? :) We can merge this PR and land a test in a separate one?

[yassinebridi]

@patak-js Can you check if that knightly build exists, it doesn't seem to be?

[patak-dev]

Ya, looks like Knightly bot is not working correctlty, back to yarn link to test then

[paraboul]

@patak-js Just added a new playground project to test various tailwind updates. The test fails without this PR's fix.

[eugene1g]

@paraboul Good playground - accurately represents the typical usage of Tailwind.

[paraboul]

I added index.css in the index.html file instead. I wasn't able to reproduce the issue in the test run by importing the css in JS. Probably related to some difference in the dependency tree specific to testing? Anyway, i was able to reproduce the issue that way.

…

On Mon, Jul 19, 2021, 13:38 patak ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In packages/playground/tailwind/src/main.js <#4267 (comment)>: > @@ -0,0 +1,6 @@ +import { createApp } from 'vue' +import App from './App.vue' +import router from './router' +// import '../index.css'; Why is this import commented? Shouldn't this be part of the tailwind setup https://tailwindcss.com/docs/guides/vue-3-vite#include-tailwind-in-your-css ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4267 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAATTVPAM5AQFWAFDPRF4WTTYQFCXANCNFSM5AN4ZODQ> .