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

feat(html): support vite-ignore attribute to opt-out of processing #18494

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 28, 2024

Description

The HTML handling code is plague with indents, so disabling whitespace when reviewing is recommended.

Supersedes and closes #11854
Fixes #11017
Fixes #8976
Fixes #3533

This new attribute should also unblock #11138


cc @leonheess

  • The reason we didn't went with fix: exclude external dependencies from html rewriting #11854 is because it relied on build.rollupOptions.external which is a Rollup-specific build option, but the feature should apply generically in dev too. (e.g. for skipping pre-transforming)
  • In that PR, we concluded to use resolve.external (which now the environment API has laid the foundation), but the option needs a bit more polish as it only works for dependencies, not relative paths.
  • Putting aside resolve.external which would've took a lot of work, this PR implements ignores for HTML only to address the linked issues for now. I think it's also helpful to ignore directly on the element instead of through a separate config.
  • I think it also makes sense to apply resolve.external for deps only for now until we have another usecase for handling relative paths.

Questions:

  1. Patak suggested to use a vite-ignore attribute instead of a comment before the HTML element. Any thoughts here?
    • One quirk I think is that vite-ignore attribute is kinda non-standard unless you prepend a data-, but the PR strips out the attribute anyways so maybe not a problem.

@bluwy bluwy added feat: html p3-significant High priority enhancement (priority) labels Oct 28, 2024
@leonheess
Copy link

I welcome any solution! Thank you so much for investing time into this!

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.

Looks good to me! Thanks for adding support for this, it will relieve a lot of users.

My vote goes to the clean vite-ignore, or if we want to make it look a bit more different than other attrs, we could go with something like __vite-ignore 🤔

Do you know of tools that could complain with non-standard attributes?

@bluwy
Copy link
Member Author

bluwy commented Oct 28, 2024

Do you know of tools that could complain with non-standard attributes?

https://validator.w3.org/ is often the one pointing out these issues, but I think as long as vite-ignore doesn't reach the browser, it could be fine. IDEs or text editor for HTML files are usually fine with it.

@patak-dev patak-dev added this to the 6.0 milestone Oct 28, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

With an attribute, it's impossible to only process some of the attributes. For example, when I have a video tag like below

<video
  src="./video.mp4"
  poster="./thumbnail.png"
/>

and only want to process the poster attribute.
But I think that is rare, and I don't have an alternative so I'm good with going with an attribute.

@patak-dev
Copy link
Member

That is a good point. I think if there is enough demand we could say vite-ignore applies to the whole tag, and vite-ignore-src only to src. But we can add this later.

@patak-dev patak-dev merged commit d951310 into main Oct 30, 2024
14 checks passed
@patak-dev patak-dev deleted the html-vite-ignore branch October 30, 2024 07:20
@loukamb
Copy link

loukamb commented Nov 12, 2024

Is there a timeline for the integration of this feature into release? Neither the current tagged version nor the 6.0.0 beta has this feature implemented. 6.0.0-beta9 seems to include it, but the feature does not appear operational.

@bluwy
Copy link
Member Author

bluwy commented Nov 12, 2024

It is already released in the beta. Can you explain what isn't working?

@loukamb
Copy link

loukamb commented Nov 12, 2024

It is already released in the beta. Can you explain what isn't working?

Ignore my comment, whoops. I wrongly attributed Vite breaking my script to it ignoring vite-ignore when in reality it was due to a malfunctioning esbuild plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: html p3-significant High priority enhancement (priority)
Projects
Archived in project
5 participants