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: move CSS emitFile logic closer to rollup #10909

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

patak-dev
Copy link
Member

Description

Continuation from:

I don't know here why we needed to recreate so much of Rollup handling for creating the asset fileName for CSS. Maybe we did it because we already had this scheme for regular assets, so we copied the same when emitting CSS assets. But now that regular assets aren't using assetFileNamesToFileName, we can also eliminate its use for CSS and delete this function together with the sanitize code @dominikg brought from Rollup.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added the p1-chore Doesn't change code behavior (priority) label Nov 13, 2022
// Add duplicate assets to the manifest
fileNameToAssetMeta.forEach(({ originalName }, fileName) => {
// Add deduplicated assets to the manifest
assets.forEach(({ originalName }, referenceId) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I made a mistake in the prev PR here, fileNameToAssetMeta doesn't have all assets. It worked because the test had a single duplicate. I can move this part to a separate PR if you think it is more clear. I consider this PR and the prev part of the same refactoring though.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused why fileNameToAssetMeta doesn't have all of assets? Above you did:

const fileNameToAssetMeta = new Map<string, GeneratedAssetMeta>()
      const assets = generatedAssets.get(config)!
      assets.forEach((asset, referenceId) => {
        const fileName = this.getFileName(referenceId)
        fileNameToAssetMeta.set(fileName, asset)
      })

so theoretically it should have all of assets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the deduplicated assets end up with the same fileName as others

Copy link
Member

Choose a reason for hiding this comment

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

Ah right we want to duplicate the assets when generating the manifest. Thanks for the explanation!

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Nov 13, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ❌ failure
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
rakkas ✅ success
svelte ⏹️ cancelled
vite-plugin-ssr ❌ failure
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

+34 −300 😲

@patak-dev patak-dev merged commit 92a206b into main Nov 14, 2022
@patak-dev patak-dev deleted the refactor/simplify-emit-css-asset branch November 14, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants