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: only run build-html plugin on bundler inputs (closes #4067) #5342

Merged
merged 6 commits into from
Jan 11, 2022
Merged

fix: only run build-html plugin on bundler inputs (closes #4067) #5342

merged 6 commits into from
Jan 11, 2022

Conversation

dimfeld
Copy link
Contributor

@dimfeld dimfeld commented Oct 18, 2021 •
edited
Loading

Description

As described in #4067, the build-html plugin runs on any file whose name ends in .html. This causes issues when using something like rollup-plugin-string to include HTML files in another way. Also see #5332 for why the ?raw suffix isn't always an option, such as when using glob imports.

This PR changes the plugin to only transform the HTML files present in the input arguments to buildStart.

Closes #4067

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.

@dimfeld dimfeld marked this pull request as ready for review October 19, 2021 07:59
AlonMiz
AlonMiz reviewed Nov 3, 2021
View reviewed changes
packages/vite/src/node/plugins/html.ts
},

async transform(html, id) {
if (id.endsWith('.html')) {
if (inputFiles.has(id)) {
Copy link

Choose a reason for hiding this comment

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

so. in case there's already a plugin, it won't be added to the "bundler inputs"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean. Is it not safe to use the list of inputs in buildStart as the list of inputs?

AlonMiz
AlonMiz previously approved these changes Nov 3, 2021
View reviewed changes
Copy link

@AlonMiz AlonMiz left a comment

Choose a reason for hiding this comment

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

looks pretty good. thanks!

@Shinigami92 Shinigami92 added feat: html needs rebase p3-minor-bug An edge case that only affects very specific usage (priority) labels Nov 3, 2021
@dimfeld
Copy link
Contributor Author

dimfeld commented Nov 3, 2021

Rebased

@Shinigami92
Copy link
Member

Seems something failed hard on windows 🤔
Maybe you need to normalize the id or so?

@dimfeld
Copy link
Contributor Author

dimfeld commented Nov 16, 2021

Seems something failed hard on windows 🤔 Maybe you need to normalize the id or so?

@Shinigami92 Looks like that worked. Thanks for the tip!

Shinigami92
Shinigami92 previously approved these changes Nov 16, 2021
View reviewed changes
@AlonMiz
Copy link

AlonMiz commented Nov 29, 2021

@dimfeld can i help somehow?

@dimfeld
Copy link
Contributor Author

dimfeld commented Nov 29, 2021

Don’t know, I thought this was all ready to merge so not sure why nobody has merged it. I guess some other PR had caused a conflict since this was approved so I can fix that later this week…

@dimfeld
Copy link
Contributor Author

dimfeld commented Nov 29, 2021

Ok, merge conflicts fixed!

@AlonMiz
Copy link

AlonMiz commented Dec 12, 2021

@yyx990803 can we merge this?

patak-dev
patak-dev approved these changes Dec 12, 2021
View reviewed changes
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.

Hey @AlonMiz, sorry for the delay. 2.7 took us a lot of our attention away :)

LGTM, thanks @nzyoni for the fix 👍🏼

I will bring this one to next team meeting before merging, just to be sure that there aren't unintended consequences here

@patak-dev patak-dev merged commit 7541a8d into vitejs:main Jan 11, 2022
@dimfeld dimfeld deleted the build-html-check-inputs branch January 11, 2022 21:58
@patak-dev
Copy link
Member

Thanks again for the PR and digging into this issue @dimfeld @AlonMiz, we'll release this fix as part of Vite 2.8.

IanVS added a commit to IanVS/vite that referenced this pull request Feb 1, 2022
@IanVS
Revert "fix: only run build-html plugin on bundler inputs (fix vitejs…
c7ba1dd 
…#4067) (vitejs#5342)"

This reverts commit 7541a8d.
@IanVS IanVS mentioned this pull request Feb 1, 2022
Merged
9 tasks
patak-dev pushed a commit that referenced this pull request Feb 2, 2022
@IanVS
fix: revert #5342, only run build-html plugin on bundler inputs (#6715)
59f8a63 
This reverts commit 7541a8d.
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

@AlonMiz AlonMiz AlonMiz approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@nzyoni nzyoni nzyoni approved these changes

Assignees
No one assigned
Labels
feat: html 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.

[vite:build-html] Unable to parse html in build step
5 participants
@dimfeld @Shinigami92 @AlonMiz @patak-dev @nzyoni