fix: use bundled css-tree and csso to avoid patch.json resolution in …#2197
Open
fix: use bundled css-tree and csso to avoid patch.json resolution in …#2197
Conversation
…bundlers css-tree and csso load patch.json via dynamic require at runtime. Bundlers like Rollup/esbuild (used by Nuxt/Nitro, Next/Vercel) cannot statically analyse this and exclude the JSON file from bundled output, causing runtime errors: Cannot find module '../data/patch.json' This fix imports from the pre-bundled dist files (css-tree/dist/csstree.esm and csso/dist/csso.esm) which have all patch data inlined, eliminating the runtime JSON require. Fixes svg#2149
|
What if instead of physically inlining the package, we reconfigure the bundler? Like here: Lines 43 to 49 in c06d8f6 what if this is changed to |
Collaborator
Actually, we don't inline the package right now and wouldn't inline the package with this PR, but that change would end up partially inlining the package. I'd expect to run into problems due to csso too. |
- Medium: The three new smoke tests at lib/svgo.test.js:395, lib/svgo.test.js:418, and lib/svgo.test.js:434 are redundant with existing coverage. Core stylesheet behavior is already covered more thoroughly in lib/style.test.js:28, lib/ style.test.js:87, and lib/style.test.js:174. Plugin fixture coverage is already wired through test/plugins/ _index.test.js:24, with direct CSS cases in test/plugins/inlineStyles.10.svg.txt:1, test/plugins/prefixIds.01.svg.txt:1, and test/plugins/minifyStyles.01.svg.txt:1. The new tests only assert toContain(...), so they add very little signal. - Medium: They also do not cover the actual regression surface. This patch is about bundled css-tree/csso resolution, but those tests run against source optimize(), not the built browser bundle. The existing browser smoke at test/browser.js:35 still uses plugins: [] on an SVG with no <style>, so it never touches the wrapped imports. If you want new coverage for this fix, it should be a CSS-bearing case in test/browser.js / test:bundles, not in lib/svgo.test.js.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…bundlers
css-tree and csso load patch.json via dynamic require at runtime. Bundlers like Rollup/esbuild (used by Nuxt/Nitro, Next/Vercel) cannot statically analyse this and exclude the JSON file from bundled output, causing runtime errors:
Cannot find module '../data/patch.json'
This fix imports from the pre-bundled dist files (css-tree/dist/csstree.esm and csso/dist/csso.esm) which have all patch data inlined, eliminating the runtime JSON require.
Fixes #2149