refactor: move CSS emitFile logic closer to rollup #10909
Conversation
| // Add duplicate assets to the manifest | ||
| fileNameToAssetMeta.forEach(({ originalName }, fileName) => { | ||
| // Add deduplicated assets to the manifest | ||
| assets.forEach(({ originalName }, referenceId) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Because the deduplicated assets end up with the same fileName as others
There was a problem hiding this comment.
Ah right we want to duplicate the assets when generating the manifest. Thanks for the explanation!
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
Description
Continuation from:
I don't know here why we needed to recreate so much of Rollup handling for creating the asset
fileNamefor 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 usingassetFileNamesToFileName, 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?