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

refactor: use rollup hashing when emitting assets #10878

Merged
merged 6 commits into from Nov 13, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Nov 11, 2022

Done: This PR depends on rollup/rollup#4712, Rollup 3.3 includes it and we updated it in a separate PR.

Description

Rollup 3 has a new hashing algorithm that fixes the issues we had when emitting assets before:

We currently do our own hashing and deduplication, see:

    // emit as asset
    // rollup supports `import.meta.ROLLUP_FILE_URL_*`, but it generates code
    // that uses runtime url sniffing and it can be verbose when targeting
    // non-module format. It also fails to cascade the asset content change
    // into the chunk's hash, so we have to do our own content hashing here.
    // https://bundlers.tooling.report/hashing/asset-cascade/
    // https://github.com/rollup/rollup/issues/3415

This is the first PR in a series to try to move back some of the responsibilities to Rollup. This should help to be compatible with external plugins using emitFile or config settings like assetFileNames. There was actually a faulty test in workers that was not letting us see a bug for workers and assetFileNames.

Ideally, we should try to move to also use import.meta.ROLLUP_FILE_URL_referenceId instead of our __VITE_ASSET__hash__. I still don't know if it is feasible though because we still need our own scheme for references in CSS. This PR already moves from our own hash to using referenceId though, and future PRs may move us closer to at least using rollup's scheme in JS files.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev marked this pull request as draft November 11, 2022 11:30
@benmccann
Copy link
Collaborator

SvelteKit and Rollup have one file naming scheme, but Vite has a different file naming scheme for inserting the hash into the file names (I don't recall the difference exactly, but think it was something like - vs . for separating the name and hash in the filename). It would be nice if Vite were able to align with Rollup as part of this as it would allow for a minor simplification in the Vite PWA codebase.

@patak-dev
Copy link
Member Author

@benmccann we are using name.[hash].[ext] for JS, CSS, assets, while the default for both rollup and esbuild is name-[hash].[ext]. I'll check with the team what was the rationale here, it may just be historical and I agree this is a good opportunity to align with rollup. It would probably be done in a different PR though.

@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Nov 12, 2022
@patak-dev patak-dev marked this pull request as ready for review November 12, 2022 10:11
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.

Some nitpicks but other looks good to me!

packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/manifest.ts Show resolved Hide resolved
Co-authored-by: 翠 / green <green@sapphi.red>
@userquin
Copy link
Contributor

userquin commented Nov 13, 2022

I don't recall the difference exactly, but think it was something like - vs .

Here the entry: https://github.com/vite-pwa/sveltekit/blob/main/src/config.ts#L53

This will go to Vite 4? We can break existing plugins, at least Vite PWA, we're forcing to detect Vite version?

EDIT: PR sent to PWA Plugin to support both layouts, also sent PR to kit integration to remove it

@patak-dev patak-dev merged commit 78c77be into main Nov 13, 2022
@patak-dev patak-dev deleted the refactor/use-rollup-assets-references branch November 13, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants