-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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: fileToBuiltUrl got undefined when file type is .ico
#7106
Conversation
@@ -33,6 +33,9 @@ const emittedHashMap = new WeakMap<ResolvedConfig, Set<string>>() | |||
export function assetPlugin(config: ResolvedConfig): Plugin { | |||
// assetHashToFilenameMap initialization in buildStart causes getAssetFilename to return undefined | |||
assetHashToFilenameMap.set(config, new Map()) | |||
|
|||
// add own dictionary by directly assigning mrmine | |||
mrmime.mimes['ico'] = 'image' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should open an issue or PR to https://github.com/lukeed/mrmime first? @lukeed may be able to do a release quick enough so we don't need to patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was an intentional part of the pkg design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you add a comment with a link to that issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluwy added 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ico is officially the image/vnd.microsoft.icon
mime type and also has had image/x-icon recognized, but both are vendor-specific and/or experimental so mrmime does not include them. The image or image/icon solutions are all userland names and not officially true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference @lukeed 👍🏼
IIUC, it would be better to use image/x-icon
instead of image
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ya, but mrmime only contains the official mimes. Everything else is customizable
Description
fix: #7083
Additional context
lukeed/mrmime#3
add own dictionary by directly assigning mrmine in assets plugin setup.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).