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

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

Merged
merged 12 commits into from Jul 20, 2021

Conversation

@paraboul
Copy link
Contributor

@paraboul paraboul commented Jul 15, 2021

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-js
Copy link
Member

@patak-js patak-js commented Jul 16, 2021

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

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

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

and only call createFileOnlyEntry here
[...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
Copy link
Contributor Author

@paraboul paraboul commented Jul 16, 2021

@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-js
Copy link
Member

@patak-js patak-js commented Jul 16, 2021

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

[...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
Copy link
Contributor Author

@paraboul paraboul commented Jul 16, 2021

@patak-js Ok I get it.

Thanks!

@OneNail
Copy link
Collaborator

@OneNail OneNail commented Jul 16, 2021

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

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

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

and only call createFileOnlyEntry here

[...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 paraboul changed the title fix(vite): ensureEntryFromUrl should return an existing module with the same id. (fix #4150) fix(vite): CSS plugint transform should call createFileOnlyEntry() for non CSS files (fix #4150) Jul 16, 2021
@paraboul
Copy link
Contributor Author

@paraboul paraboul commented Jul 16, 2021

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

@paraboul paraboul changed the title fix(vite): CSS plugint transform should call createFileOnlyEntry() for non CSS files (fix #4150) fix(vite): CSS plugin transform should call createFileOnlyEntry() for non CSS files (fix #4150) Jul 16, 2021
@paraboul
Copy link
Contributor Author

@paraboul paraboul commented Jul 16, 2021

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

@OneNail
Copy link
Collaborator

@OneNail OneNail commented Jul 16, 2021

@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-js
Copy link
Member

@patak-js patak-js commented Jul 16, 2021

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

@patak-js
Copy link
Member

@patak-js patak-js commented Jul 16, 2021

@knightly-bot build this

@knightly-bot
Copy link

@knightly-bot knightly-bot commented Jul 16, 2021

Nightly Build

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

📦 npm package

@patak-js
Copy link
Member

@patak-js patak-js commented Jul 16, 2021

@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
Copy link

@eugene1g eugene1g commented Jul 16, 2021

@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
Copy link

@eugene1g eugene1g commented Jul 16, 2021

@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
Copy link
Contributor Author

@paraboul paraboul commented Jul 16, 2021

@yassinebridi
Copy link

@yassinebridi yassinebridi commented Jul 16, 2021

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

@patak-js
Copy link
Member

@patak-js patak-js commented Jul 16, 2021

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

@paraboul
Copy link
Contributor Author

@paraboul paraboul commented Jul 17, 2021

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

@paraboul paraboul requested a review from patak-js Jul 17, 2021
@eugene1g
Copy link

@eugene1g eugene1g commented Jul 17, 2021

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

@paraboul paraboul requested a review from Shinigami92 Jul 18, 2021
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/moduleGraph.ts Outdated Show resolved Hide resolved
@paraboul
Copy link
Contributor Author

@paraboul paraboul commented Jul 19, 2021

paraboul and others added 3 commits Jul 19, 2021
Co-authored-by: patak <matias.capeletto@gmail.com>
Co-authored-by: patak <matias.capeletto@gmail.com>
Co-authored-by: patak <matias.capeletto@gmail.com>
@paraboul paraboul requested a review from patak-js Jul 19, 2021
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
@paraboul paraboul requested a review from patak-js Jul 20, 2021
@patak-js patak-js changed the title fix(vite): CSS plugin transform should call createFileOnlyEntry() for non CSS files (fix #4150) fix: call createFileOnlyEntry() only for CSS deps, fix #4150 Jul 20, 2021
@patak-js patak-js merged commit 89e3160 into vitejs:main Jul 20, 2021
6 checks passed
6 checks passed
@github-actions
Build&Test: node-12, ubuntu-latest
Details
@github-actions
Build&Test: node-14, ubuntu-latest
Details
@github-actions
Build&Test: node-16, ubuntu-latest
Details
@github-actions
Build&Test: node-14, macos-latest
Details
@github-actions
Build&Test: node-14, windows-latest
Details
@github-actions
Lint: node-14, ubuntu-latest
Details
ElMassimo added a commit to ElMassimo/vite that referenced this pull request Aug 12, 2021
The change in vitejs#4267 caused all CSS
dependencies of files that are not CSS (Tailwind CSS JIT deps) to be
tracked with the `base` prefix in the module graph.

URLs in the module graph are normalized and are never prefixed with base
and as a result changes to those files were not causing the CSS module
to be invalidated.

See tailwindlabs/tailwindcss#4978 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants