feat(html,asset): improve asset resolution #7643
Conversation
I didn't look further into it but if needed/wanted I can implement tests to cover the tagged issues. |
| filter: (attribute, value, node) => { | ||
| const relProp = findProp(node, 'rel') | ||
| if ( | ||
| relProp && | ||
| relProp.type === NodeTypes.ATTRIBUTE && | ||
| relProp.value?.content && | ||
| [ | ||
| 'stylesheet', | ||
| 'icon', | ||
| 'shortcut icon', | ||
| 'mask-icon', | ||
| 'apple-touch-icon', | ||
| 'apple-touch-icon-precomposed', | ||
| 'apple-touch-startup-image', | ||
| 'manifest', | ||
| 'prefetch', | ||
| 'preload' | ||
| ].includes(relProp.value.content) | ||
| ) { | ||
| return true | ||
| } | ||
|
|
||
| const itempropProp = findProp(node, 'itemprop') | ||
| if ( | ||
| itempropProp && | ||
| itempropProp.type === NodeTypes.ATTRIBUTE && | ||
| itempropProp.value?.content && | ||
| [ | ||
| 'image', | ||
| 'logo', | ||
| 'screenshot', | ||
| 'thumbnailurl', | ||
| 'contenturl', | ||
| 'downloadurl', | ||
| 'duringmedia', | ||
| 'embedurl', | ||
| 'installurl', | ||
| 'layoutimage ' | ||
| ].includes(itempropProp.value.content) | ||
| ) { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
I think this can also be configured here. I don't know why it should be turned into a method. I would appreciate it if you could tell me
{
rel: ['stylesheet','icon','shortcut icon','mask-icon','apple-touch-icon','apple-touch-icon-precomposed','apple-touch-startup-image','manifest','prefetch','preload'],
itemprop: ['image','logo','screenshot','thumbnailurl','contenturl','downloadurl','duringmedia','embedurl','installurl','layoutimage']
}There was a problem hiding this comment.
I didn't give much thought to how it should be configured. I basically replicated the API from html-loader, as proposed by #7362 (comment). Furthermore, the approach is most likely a matter of personal preference. If the array-based configuration fits better into vites config schema then that's how it should be done, so as not to create unnecessary complexity where it isn't required.
There was a problem hiding this comment.
Since the processing I've seen is all the same, I'm guessing it can be done this way. 🫣 This reduces a lot of code.
There was a problem hiding this comment.
I would also prefer we simplify the code. But the functional form would allow for a more flexible configuration point since the PR is exposing build.htmlAssetSources. I'm not sure we would like to expose this complexity it is better to leave it to userland. Maybe we could only correct the list as a first step. Added the issue for discussion in a future team meeting.
There was a problem hiding this comment.
From my experience building svelte-preprocess-import-assets, this is actually the only way to ensure correctness (with a filter). I tried simplifying it but it isn't quite achievable if we follow the html spec.
There was a problem hiding this comment.
since the PR is exposing
build.htmlAssetSources
It doesn't look like the PR expose it now (maybe I've missed a discussion) 🤔
There was a problem hiding this comment.
Ya, the change to remove it was force pushed. We can still discuss it in the meeting, the PR was exposing build.htmlAssetSources and letting you define whatever you want instead of the default defaultHtmlAssetSources array. The API was exactly the one of that array.
There was a problem hiding this comment.
Sorry, my mistake. I discovered some old incorrectly merged changes while rebasing the branch earlier. However, I have the changes that expose build.htmlAssetSources saved locally. I'll commit them later so that the discussion has a point of reference.
bluwy
added
p2-nice-to-have
Add logic for selecting source attributes and extend build options to allow for configuration.
| } | ||
| } | ||
|
|
||
| export const defaultHtmlAssetSources: HtmlAssetSource[] = [ |
There was a problem hiding this comment.
I think it would be great to extract this and it's related types/functions as it's own file as this is HUGE. Maybe we can put it in the src/node folder
|
Is there any update on this? Do you need some support for this? |
|
There's a merge conflict for this PR that would need to be resolved |
|
This looks great. Are you still willing to carry this forward, @grievouz? We would love to ship this in Vite 4. |
|
Looks like we had discussed this in a couple meetings ago and forgot to give an update. We think this feature is great for Vite 4, but should be shipped without an option first. |
|
Closing this one for now in favour of #11138 |
Description
Expand on the default set of tags to convert into imports:
Closes Add import directive for html attributes #7362
Closes Custom html attributes resolving #5098
Use Rollup to resolve import urls (edit by @patak-dev: To be done in a separate PR)
Aliases do not work for preload links in index.html #3000
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).