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.

Sign up for GitHub

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

Jump to bottom

fix(hmr): html registered as a PostCSS dependency, fix #3716 #4127

Merged
merged 1 commit into from
Jul 6, 2021
Merged

fix(hmr): html registered as a PostCSS dependency, fix #3716 #4127

merged 1 commit into from
Jul 6, 2021

Conversation

y1d7ng
Copy link
Contributor

@y1d7ng y1d7ng commented Jul 5, 2021

Description

fix #3716

Additional context

PostCSS plugin Tailwind JIT register any file as a dependency to a CSS file. When using <link rel="stylesheet">, index.html module's importers includes style.css module, the browser does not automatically refresh as index.html is changed.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <title>Vite App</title>
    <link rel="stylesheet" href="/style.css" />
  </head>
  <body>
    <div id="app">
      <h1>Hello Vite!</h1>
    </div>
  </body>
</html>

The current solution does not include the processing of direct import css:

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

Line 252 in e30ce56

[...node.importers].every((i) => isCSSRequest(i.url))

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

Lines 101 to 102 in e30ce56

export const isCSSRequest = (request: string): boolean =>
cssLangRE.test(request) && !directRequestRE.test(request)

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.

@y1d7ng y1d7ng mentioned this pull request Jul 5, 2021
Closed
@Shinigami92 Shinigami92 added feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) labels Jul 5, 2021
@patak-dev
Copy link
Member

Checking the naming of these helpers

export const isCSSRequest = (request: string): boolean =>
  cssLangRE.test(request) && !directRequestRE.test(request)

export const isDirectCSSRequest = (request: string): boolean =>
  cssLangRE.test(request) && directRequestRE.test(request)

That are used in some places like

   if (
        isJSRequest(url) ||
        isImportRequest(url) ||
        isCSSRequest(url) ||
        isHTMLProxy(url)
      ) {
        // for CSS, we need to differentiate between normal CSS requests and imports
        if (isCSSRequest(url) && req.headers.accept?.includes('text/css')) {
          url = injectQuery(url, 'direct')
        }

I wonder if there are other bugs like this because people were using it without knowing that it explicitly tests for not being a direct request.

I think we should have

export const isCSSRequest = (request: string): boolean =>
  cssLangRE.test(request)

// Only use this helper in plugins when the URL has already being marked
export const isIndirectCSSRequest = (request: string): boolean =>
  isCSSRequest(request) && !directRequestRE.test(request)

export const isDirectCSSRequest = (request: string): boolean =>
  cssLangRE.test(request) && directRequestRE.test(request)

In the check above we could use an isCSSRequest check + an explicit !directRequestRE if that is really needed at that point. After that, URLs are already marked in plugins

But this refactor shouldn't be part of this PR, so it is better to me that we go forward with this fix

@y1d7ng
Copy link
Contributor Author

y1d7ng commented Jul 6, 2021

@patak-js Thank you for your comment. isCSSRequest is indeed misleading. It may be misused and cause other bugs. I think your proposal to add isIndirectCSSRequest is very nice.

@patak-dev patak-dev changed the title fix(hmr): hmr disabled when html is registered as a PostCSS dependency fix(hmr): html registered as a PostCSS dependency, fix #3716 Jul 6, 2021
@patak-dev patak-dev merged commit 09c6c94 into vitejs:main Jul 6, 2021
@patak-dev
Copy link
Member

@patak-js Thank you for your comment.

Thank you for all the work in Vite lately!

isCSSRequest is indeed misleading. It may be misused and cause other bugs. I think your proposal to add isIndirectCSSRequest is very nice.

Would you like to give this refactor a try? Or should we open it to others?

@patak-dev patak-dev mentioned this pull request Jul 6, 2021
Closed
6 tasks
@y1d7ng
Copy link
Contributor Author

y1d7ng commented Jul 6, 2021 •
edited
Loading

@patak-js I'm glad to give this refactor a try. Thanks~

@patak-dev
Copy link
Member

@patak-js I'm glad to give this refactor a try. Thanks~

Awesome, thanks! Just a fair warning that I think it is a good idea but we will need to review it later with the rest of the team once it is on the table

@y1d7ng y1d7ng mentioned this pull request Aug 3, 2021
Merged
9 tasks
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
@y1d7ng @aleclarson
fix(hmr): html registered as a PostCSS dependency, fix vitejs#3716 (v…
50f976b 
…itejs#4127)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patak-dev patak-dev patak-dev approved these changes

@GrygrFlzr GrygrFlzr GrygrFlzr approved these changes

Assignees
No one assigned
Labels
feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTML auto reload is disabled when index.html is registered as a PostCSS dependency
4 participants
@y1d7ng @patak-dev @GrygrFlzr @Shinigami92