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(css): file or contents missing error in build watch (#3742) #3747

Merged
merged 5 commits into from
Jun 21, 2021

Conversation

javastation
Copy link
Contributor

@javastation javastation commented Jun 10, 2021

Description

Fix #3742, styles will be incomplete,because styles is initialized in buildStart.

if (styles.has(id)) {

In asset.ts, assetHashToFilenameMap initialization in buildStart causes getAssetFilename called in css.ts to return undefined.
const filename = getAssetFilename(fileHash, config) + postfix

I add another variable emittedHashesSet to determine whether to emit a file.

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.

@Shinigami92 Shinigami92 added p3-minor-bug An edge case that only affects very specific usage (priority) feat: css labels Jun 10, 2021
@Shinigami92
Copy link
Member

Could you add tests that will fail when your fix is not applied?

@javastation
Copy link
Contributor Author

Ok, you can read my repo in #3742 about styles, I will create a new branch to reproduce the problem of returning undefined.

@javastation
Copy link
Contributor Author

javastation commented Jun 10, 2021

This is repo https://github.com/javastation/vite/tree/bug

  1. clone https://github.com/javastation/vite/tree/bug and checkout bug
  2. run yarn and yarn build-vite
  3. cd ./vite/packages/playground/assets
  4. run yarn watch
  5. make some changes to index.html, you will find undefined in index.*.cssin dist
  6. if you change icons.css, emitFile will trigger multiple times

undefined

@Shinigami92
Copy link
Member

What I meant was a test in our test-suite, so e.g. a unit test or integration test in one of the playgrounds. This way it will always checked when someone create a PR.
Your reproduction repo will currently only be checked once and then pass away.
So I would like to see tests to prevent e.g. regression in the future.

@javastation
Copy link
Contributor Author

Ok, I got it wrong before.

scripts/jestPerTestSetup.ts Outdated Show resolved Hide resolved
scripts/jestPerTestSetup.ts Outdated Show resolved Hide resolved
scripts/jestPerTestSetup.ts Outdated Show resolved Hide resolved
scripts/jestPerTestSetup.ts Outdated Show resolved Hide resolved
scripts/jestPerTestSetup.ts Outdated Show resolved Hide resolved
scripts/jestPerTestSetup.ts Show resolved Hide resolved
packages/vite/src/node/plugins/asset.ts Show resolved Hide resolved
packages/playground/testUtils.ts Outdated Show resolved Hide resolved
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great first PR to the project @javastation. Thanks a lot for also taking the time to improve the test suite.

Not for this PR, but if you would like to do so, it would be great if you could do a review of other plugin caches and how they interact with build watch. I sent a few PRs trying to fix build watch a few versions ago ( #3537, #3516, #3530, #3512 ), but I didn't take into account the internal rollup cache that prevents unmodified files to go through the hooks pipeline in each build. There is also another cache in the build-html plugin (see #3535) that I couldn't modify and it may be related to the issue you found. Thanks again, I hope we keep seeing you around in the project 👍🏼

@javastation
Copy link
Contributor Author

I also noticed the html-plugin yesterday, some static assets in html will emit multiple times in build watch, cache collection is necessary, I will also try to do something for the project.

@patak-dev patak-dev changed the title fix: fix css file or contents missing error in build watch (#3742) fix(css): file or contents missing error in build watch (#3742) Jun 21, 2021
@patak-dev patak-dev merged commit 26b1b99 into vitejs:main Jun 21, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Css file or content will be lost in build watch
3 participants