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: fileToBuiltUrl got undefined when file type is .ico #7106

Merged
merged 4 commits into from Feb 27, 2022
Merged

fix: fileToBuiltUrl got undefined when file type is .ico #7106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2993580
test: ico file
poyoho Feb 26, 2022
151e8e2
feat: add own dictionary by directly assigning mrmine
poyoho Feb 27, 2022
7f0f53b
test: ico file type
poyoho Feb 27, 2022
4961a3b
chore: add mrmime issue to comment
patak-dev Feb 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/playground/assets/__tests__/assets.spec.ts
View file
Open in desktop
Expand Up @@ -120,6 +120,10 @@ describe('css url() references', () => {
const match = isBuild ? `data:image/png;base64` : `/foo/nested/icon.png`
expect(await getBg('.css-url-base64-inline')).toMatch(match)
expect(await getBg('.css-url-quotes-base64-inline')).toMatch(match)
const icoMatch = isBuild ? `data:image;base64` : `favicon.ico`
const el = await page.$(`link.ico`)
const herf = await el.getAttribute('href')
expect(herf).toMatch(icoMatch)
})

test('multiple urls on the same line', async () => {
Expand Down
Binary file added packages/playground/assets/favicon.ico
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions packages/playground/assets/index.html
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

<head>
<meta charset="UTF-8" />
<link class="ico" rel="icon" type="image/svg+xml" href="favicon.ico" />
</head>

<link rel="icon" href="data:," />
Expand Down
4 changes: 4 additions & 0 deletions packages/vite/src/node/plugins/asset.ts
View file
Open in desktop
Expand Up @@ -33,6 +33,10 @@ 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 entry by directly assigning mrmine
// https://github.com/lukeed/mrmime/issues/3
mrmime.mimes['ico'] = 'image'
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

lukeed/mrmime#3

That was an intentional part of the pkg design.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bluwy added 🙈

Copy link

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

Copy link
Member

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

Copy link

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

return {
name: 'vite:asset',

Expand Down