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

Only set asset names when finalizing #4919

Merged
merged 3 commits into from Mar 23, 2023
Merged

Conversation

lukastaegert
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

The logic to guarantee deterministic names for emitted assets with the same source added in #4912 was unfortunately breaking this.getFileName. The new logic now only fixes the name when doing the first round of finalizations before renderStart, which should be good enough. Assets emitted at a later stage will still follow a "first assets determines the name" strategy. This guarantees, that this.getFileName will always return a correct and reliable name that does not change.

@github-actions
Copy link

github-actions bot commented Mar 23, 2023

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#fix-asset-deduplication

or load it into the REPL:
https://deploy-preview-4919--rollupjs.netlify.app/repl/?pr=4919

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #4919 (680244a) into master (d44fba6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4919   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files         221      221           
  Lines        8029     8049   +20     
  Branches     2210     2214    +4     
=======================================
+ Hits         7950     7970   +20     
  Misses         26       26           
  Partials       53       53           
Impacted Files Coverage Δ
src/utils/FileEmitter.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sapphi-red
Copy link
Contributor

Thanks!

But I'm afraid this does not solve Vite's issue (#4909, vitejs/vite#11911).

Vite needs to call this.emitFile in renderChunk hook as it relies on the rollup's chunking mechanism. IIUC this PR requires this.emitFile to be called before renderStart.

I'm having a feeling that

  • deduping assets with same sources
  • able to call this.emitFile any time
  • able to call this.getFileName any time after renderStart

has a nature of non-deterministic file names.

I think I can mitigate the issue specifically for CSS files by calling this.emitFile before await, if rollup calls renderChunk hooks for each chunk in a deterministic order.

}
fileName = newFileName;
fileNamesBySource.set(sourceHash, fileName);
if (!fileName || assetFileName < fileName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The fileName sort strategy is a bit different from #4912. The previous code always uses the shortest fileName while this one doesn't; I think it's better to keep the same strategy, otherwise this would be a breaking change.

@lukastaegert lukastaegert merged commit 75c5113 into master Mar 23, 2023
14 checks passed
@lukastaegert lukastaegert deleted the fix-asset-deduplication branch March 23, 2023 08:51
@rollup-bot
Copy link
Collaborator

This PR has been released as part of rollup@3.20.1. You can test it via npm install rollup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v3.20.0 breaks Vite Storybook builds
4 participants