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: style hmr reduce replace style code #7869

Merged
merged 22 commits into from
Apr 30, 2022

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Apr 22, 2022

Description

fix: #7836
fix: #7913
fix: #6737
fix: #7952

Additional context

The previous method of replacing a style with script insertion may cause style priority issues and some style updates are invalid because middleware doesn't compile index HTML every time. So the update only needs to make the connection between CSS (including nested CSS) and Html and if HTML update will full-reload.


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.

@poyoho
Copy link
Member Author

poyoho commented Apr 22, 2022

For this error: https://github.com/vitejs/vite/runs/6131322405?check_suite_focus=true#step:9:109

@sapphi-red this PR just use the style update hmr don't render css, can I remove the

test('inline css', async () => {
const css = await getStyleTagContentIncluding('.inline ')
const map = extractSourcemap(css)
expect(formatSourcemapForSnapshot(map)).toMatchInlineSnapshot(`
Object {
"mappings": "AAGO;AACP,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;AACX,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;AACf,CAAC,CAAC,CAAC;",
"sources": Array [
"/root/index.html",
],
"sourcesContent": Array [
"<link rel=\\"stylesheet\\" href=\\"./linked.css\\" />
<link rel=\\"stylesheet\\" href=\\"./linked-with-import.css\\" />
<style>
.inline {
color: red;
}
</style>
<div class=\\"wrapper\\">
<h1>CSS Sourcemap</h1>
<p class=\\"inline\\">&lt;inline&gt;</p>
<p class=\\"linked\\">&lt;linked&gt;: no import</p>
<p class=\\"linked-with-import\\">&lt;linked&gt;: with import</p>
<p class=\\"imported\\">&lt;imported&gt;: no import</p>
<p class=\\"imported-with-import\\">&lt;imported&gt;: with import</p>
<p class=\\"imported-sass\\">&lt;imported sass&gt;</p>
<p class=\\"imported-sass-module\\">&lt;imported sass&gt; with module</p>
<p class=\\"imported-less\\">&lt;imported less&gt; with string additionalData</p>
<p class=\\"imported-stylus\\">&lt;imported stylus&gt;</p>
</div>
<script type=\\"module\\">
import './imported.css'
import './imported-with-import.css'
import './imported.sass'
import sassModule from './imported.module.sass'
document
.querySelector('.imported-sass-module')
.classList.add(sassModule['imported-sass-module'])
import './imported.less'
import './imported.styl'
</script>
<iframe src=\\"virtual.html\\"></iframe>
",
],
"version": 3,
}
`)
})
I think it don't need to sourcemap?

@sapphi-red
Copy link
Member

I think it is ok to remove that test.

@sapphi-red
Copy link
Member

sapphi-red commented Apr 27, 2022

It looks like URL rebasing is not working.

<style>
  .wrapper {
    background: url('./ok.png');
  }
</style>

Also other transforms are not working, but maybe these are expected.

<style>
  .wrapper {
    .linked {
      color: blue;
    }
  }
</style>

These does not work during dev, but it does work after build.

poyoho and others added 2 commits April 27, 2022 09:57
Co-authored-by: 翠 / green <green@sapphi.red>
@poyoho
Copy link
Member Author

poyoho commented Apr 27, 2022

@sapphi-red

<h2>url in style tag</h2>
<h3>url</h3>
<style class="style-url">
.style-url-assets {
background: url('./nested/asset.png');
background-size: 10px 10px;
}
</style>
<div style="background: url('./nested/asset.png'); background-size: 10px 10px">
inline style
</div>
<div class="style-url-assets">use style class</div>
This case had unit test for it. I should add the test assertion.

It looks like URL rebasing is not working.

<style>
  .wrapper {
    background: url('./ok.png');
  }
</style>

I think the below code is expected. css also no support this syntax.

Also other transforms are not working, but maybe these are expected.

<style>
  .wrapper {
    .linked {
      color: blue;
    }
  }
</style>

These does not work during dev, but it does work after build.

@sapphi-red
Copy link
Member

This case had unit test for it. I should add the test assertion.

This works when I access / but it does not work when I access /foo/.

@poyoho
Copy link
Member Author

poyoho commented Apr 27, 2022

@sapphi-red @bluwy @patak-dev compileCSSHTML Now it seems that this method is the best solution. because indexHtmlTransform hook in dev can't replace flag like build mode. Replace it intime it seems well.

if use the prev way and make the ?html-proxy&1.css reload will break css priority.

if use script patch it, the first screen is used unexpected css.

I don't know if there is any other better way 🤔

@patak-dev
Copy link
Member

@poyoho I was also thinking the same while reviewing this PR, I forgot you already explored compiling the inline CSS before. +1 to revisit that approach.

@poyoho
Copy link
Member Author

poyoho commented Apr 27, 2022

@poyoho I was also thinking the same while reviewing this PR, I forgot you already explored compiling the inline CSS before. +1 to revisit that approach.

now all the request css is go to replace the raw css. CSS request can reduce coupled between html and css plugin. But now css request seems to no work well.

@poyoho
Copy link
Member Author

poyoho commented Apr 28, 2022

#6859 is seems not a bug in vite

poyoho and others added 2 commits April 28, 2022 20:40
Co-authored-by: patak <matias.capeletto@gmail.com>
@patak-dev
Copy link
Member

This is a great direction 43c3dce!

@poyoho
Copy link
Member Author

poyoho commented Apr 28, 2022

css request had too many bug, let clean all the css request in this time. 🤦

@poyoho poyoho marked this pull request as ready for review April 28, 2022 15:26
patak-dev
patak-dev previously approved these changes Apr 28, 2022
@patak-dev
Copy link
Member

I think we should merge this PR and test it against vite-ecosystem-ci. I hope this is the last piece to stabilize 2.9, awesome work here @poyoho.

@sapphi-red
Copy link
Member

sapphi-red commented Apr 28, 2022

I found a bug. (an edge case)

  1. Run css playground (pnpm run dev) and open http://localhost:3000/foo/ (not /).
  2. Add <style>.wrapper{ font-weight: bold; }</style> to index.html.
  3. [vite] page reload index.html is output. But style not applied.
  4. After manual page reload, the style is applied.

@sapphi-red
Copy link
Member

sapphi-red commented Apr 28, 2022

It was not only with css.

  1. Run css playground. (pnpm run dev) and open http://localhost:3000/foo/ (not /).
  2. Change <h1>CSS</h1> to <h1>CSSfoo</h1> in index.html.
  3. [vite] page reload index.html is output. But content is not updated.
  4. After manual page reload, the content is updated.

@sapphi-red sapphi-red added feat: css p2-edge-case Bug, but has workaround or limited in scope (priority) labels Apr 28, 2022
@poyoho
Copy link
Member Author

poyoho commented Apr 29, 2022

how about removing the judge?

pagePath === payloadPath ||
(pagePath.endsWith('/') && pagePath + 'index.html' === payloadPath)

when pagePath=/foo/ and payloadPath=index.html will don't reload page. The judge makes no support no base on base url hmr.

@sapphi-red
Copy link
Member

I think base is supported by this line. payloadPath becomes /foo/index.html if base is /foo/.

const payloadPath = base + payload.path.slice(1)

I feel it is ok to reload regardless to pagePath when payload.path is index.html.

@poyoho
Copy link
Member Author

poyoho commented Apr 29, 2022

It is?

if (payloadPath === '/index.html') page.reload()

I think so. How about you @patak-dev 👀

@patak-dev
Copy link
Member

@poyoho @sapphi-red I think we should create a new issue for that problem, as it is unrelated to this PR. It is strange we didn't get more reports for this issue before.

Let's focus on the issue being solved in this PR here.

@sapphi-red sapphi-red mentioned this pull request Apr 29, 2022
7 tasks
@patak-dev patak-dev merged commit a30a548 into vitejs:main Apr 30, 2022
@poyoho poyoho deleted the fix/indexhtml-update branch April 30, 2022 04:31
@poyoho poyoho mentioned this pull request May 4, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css feat: hmr p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
3 participants