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 more asset sources #11138

Merged
merged 10 commits into from
Oct 31, 2024
Merged

feat(html)!: support more asset sources #11138

merged 10 commits into from
Oct 31, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 1, 2022

Description

Support more tags to process as assets.

Close #7362
Close #5098

Supersedes and closes #7643 (There was a lot of conflicts that it's easier to start from scratch)

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.

packages/vite/src/node/assetSource.ts Outdated Show resolved Hide resolved
packages/vite/src/node/assetSource.ts Outdated Show resolved Hide resolved
packages/vite/src/node/assetSource.ts Outdated Show resolved Hide resolved
@sapphi-red sapphi-red added feat: html p2-nice-to-have Not breaking anything but nice to have (priority) labels Dec 3, 2022
@sapphi-red sapphi-red added this to the 4.0 milestone Dec 5, 2022
@bluwy bluwy modified the milestones: 4.0, 5.0 Dec 7, 2022
@bluwy
Copy link
Member Author

bluwy commented Dec 7, 2022

Moving to the 5.0 milestone as we don't have a way to opt-out of this feature if someone needs to. Either a dedicated API, sharing with build.rollupOptions.external, <!-- @vite-ignore -->, etc which needs some discussion.

@septatrix
Copy link

Moving to the 5.0 milestone as we don't have a way to opt-out of this feature if someone needs to. Either a dedicated API, sharing with build.rollupOptions.external, <!-- @vite-ignore -->, etc which needs some discussion.

Would ?url not work as a opt-out on a per-URL basis?

I am really eager to have this feature as early as possible as it just makes Vite so much more suited for SSG. Not only image transformation as I brought up a few days ago (#11099) but also CSP hashes. Both of which are recommended for modern websites and currently missing in Vite whereas other SSG solutions support them like this eleventy blog theme.

@bluwy
Copy link
Member Author

bluwy commented Dec 10, 2022

Would ?url not work as a opt-out on a per-URL basis?

We can't ask the user to update their links to opt-out. The links should stay as is.

We'd need some time to discuss the API before we can move forward with this. (Related discussion: #3533)

@ntarbie
Copy link

ntarbie commented Dec 16, 2022

What about making it opt in? Similar to assetsInclude, an optional config for the attribute strings you need to locate and parse assets in?

@riw
Copy link

riw commented Dec 20, 2022

+1
Would be nice to have this option not only for assets but for js files, too (we use a library that lazy loads a js file from a data attribute when the element enters the viewport)

@Maytha8
Copy link

Maytha8 commented Feb 25, 2023

Moving to the 5.0 milestone as we don't have a way to opt-out of this feature if someone needs to. Either a dedicated API, sharing with build.rollupOptions.external, <!-- @vite-ignore -->, etc which needs some discussion.

Would making a new configuration option for the Vite config file fix this issue?

export interface UserConfig {
  // ...
  /**
   * Configure which HTML tags and attributes to use as asset sources.
   */
  assetSources?: AssetSourcesOptions
  // ...
}
export interface AssetSourcesOptions {
  [tag: string]: {
    [attribute: string]: boolean
  }
}

Default configuration would be: (this matches what is currently present in the code)

// ...
assetSources: {
  audio: {src: false},
  embed: {src: false},
  img: {src: true, srcset: true},
  input: {src: false},
  object: {src: false},
  source: {src: true, srcset: true},
  track: {src: false},
  video: {poster: true, src: true},
  image: {href: true, 'xlink:href': true},
  use: {href: true, 'xlink:href': true},
  link: {href: true, imagesrcset: false},
  meta: {content: false},
}
// ...

I'm new to contributing to pull requests, so forgive me if I do/say something dumb.

Copy link

@Maytha8 Maytha8 left a comment

Choose a reason for hiding this comment

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

I think it's a much cleaner implementation of finding asset sources in HTML than what is currently in the codebase.

fdbeirao added a commit to CodeFab-io/homepage that referenced this pull request Apr 20, 2023
Can't add the og:image yet due to vitejs/vite#11138 😔
@bluwy
Copy link
Member Author

bluwy commented May 14, 2023

Would making a new configuration option for the Vite config file fix this issue?

Sorry for not getting back, this slipped through my notifications 😅 This would indeed be a solution too but ideally we want to keep the configuration internally until needed.


In different news, in the last meeting we discussed that we would introduce resolve.external (#6582) as a way to externalize things in dev and build. And ideally it would cover HTML too (similar PR: #11854).

I'm still (secretly) hoping we would support <!-- @vite-ignore --> because it feels convenient in HTML files, but we can improve this later in a non-breaking way.

@bluwy
Copy link
Member Author

bluwy commented Oct 18, 2023

Dropping this off the milestone for now. We can revisit this in the next minor. Further context: #11854 (comment)

@bluwy bluwy removed this from the 5.0 milestone Oct 18, 2023
@rotu
Copy link

rotu commented Dec 22, 2023

Would love to see this landed. #7062 is very unexpected and feels like a bug. I can see how there's space for adding more build options. But treating <img href="foo.svg"> so differently from <object data="foo.svg"> for bundling with Vite is very confusing.

@bluwy bluwy marked this pull request as draft October 29, 2024 12:57
@bluwy
Copy link
Member Author

bluwy commented Oct 29, 2024

Will need to wait for #18494 before I can properly rebase the code. Not sure if this will get into the next major though, but it's close to done

@patak-dev patak-dev added this to the 6.0 milestone Oct 30, 2024
Comment on lines -280 to +279
getAttrKey(src) === 'srcset',
/* useSrcSetReplacer */ false,
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized the script tag src is never going to be a srcset, so this can be false

@bluwy bluwy marked this pull request as ready for review October 30, 2024 16:11
@bluwy
Copy link
Member Author

bluwy commented Oct 30, 2024

This is now done. Compared to the initial implementation, I shrunk down the complexity of handled HTML elements list quite a lot. I realized we don't have to completely follow what html-loader has:

  • If the link tag has a href, it's always safe to assume that's being used as a URL, so we always process it (same as previous behaviour). html-loader added rel and itemprop checks which I don't think is needed.
  • link with imagesrcset if used is almost always guaranteed a srcset that can be safe to process, we don't have to check other attributes.
  • meta with itemprop also shouldn't change the meaning of content being a URL or not. From what I can tell, itemprop is merely additional property information and doesn't change the meta tag intent.

There may be actual reasons these were implemented in html-loader that I missed. I tried searching for links or comments for where these behaviour can be found, but I couldn't find any.

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.

LGTM 👍

@sapphi-red
Copy link
Member

It would be nice to mention it in the migration guide. That more attributes are now transformed and it can be excluded by vite-ignore attribute.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 1625d98: Open

suite result latest scheduled
analogjs failure success
astro failure success
histoire failure success
redwoodjs failure failure
remix failure failure
storybook failure success
sveltekit failure failure
unocss failure success
vike failure failure
vite-plugin-vue failure success
vitest failure failure

ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-setup-catalogue, vitepress, vuepress

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.

awesome work

@patak-dev patak-dev merged commit 8a7af50 into main Oct 31, 2024
16 checks passed
@patak-dev patak-dev deleted the asset-source branch October 31, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feat: html p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add import directive for html attributes Custom html attributes resolving
8 participants