Skip to content

fix(optimizer): handle more chars that will be sanitized#22208

Merged
sapphi-red merged 3 commits intovitejs:mainfrom
sapphi-red:fix/optimize-deps-handle-more-chars-that-will-be-sanitized
Apr 20, 2026
Merged

fix(optimizer): handle more chars that will be sanitized#22208
sapphi-red merged 3 commits intovitejs:mainfrom
sapphi-red:fix/optimize-deps-handle-more-chars-that-will-be-sanitized

Conversation

@sapphi-red
Copy link
Copy Markdown
Member

Since $ is also sanitized by Rolldown by sanitizeFileName, the filename does not contain $.
#21886 fixed a similar issue for +, but it was not a general fix. In this PR, I've rewritten the flattenId function to escape any characters that will be escaped by the default sanitizeFileName.

close #22198

@sapphi-red sapphi-red added p2-edge-case Bug, but has workaround or limited in scope (priority) feat: deps optimizer Esbuild Dependencies Optimization labels Apr 10, 2026
Comment thread playground/js-sourcemap/__tests__/js-sourcemap.spec.ts Outdated
Comment thread packages/vite/src/node/utils.ts Outdated
@afurm
Copy link
Copy Markdown

afurm commented Apr 11, 2026

The approach of aligning flattenId with sanitizeFileName is the right direction — handling a class of invalid path chars instead of enumerate-and-replace is more maintainable.

One concern: flattenId escapes _ to __ before calling sanitizeFileName, but then sanitizeFileName decodes __2f_ and __2b+. This means a dependency ID that legitimately contains the substring _2f (e.g. my_2f_lib) would after pre-bundling produce the same flattened ID as my/lib. Does sanitizeFileName have a mechanism to avoid this ambiguity, or is there an ordering issue where flattenId needs to run after sanitizeFileName rather than before?

Copy link
Copy Markdown
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.

I checked and I think this should invalidate the metadata hash so it's reoptimized on Vite update. Otherwise it might still refer to the old paths.

Comment thread packages/vite/src/node/utils.ts Outdated
Comment on lines +76 to +83
.replaceAll('_', '__')
// replace any characters that will be replaced by sanitizeFileName
.replace(invalidUrlPathCharRE, (c) => '_' + c.charCodeAt(0).toString(16))
.replace(
additionalFlattenIdCharRE,
(c) => '_' + c.charCodeAt(0).toString(16),
)
.replace(replaceNestedIdRE, '__'),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _2f pattern here makes it harder to read personally. I understand making it robust, but maybe we can go with a middleground:

What if we do the normal underscore replacements for the commons chars, like / and ., and then do the _2f pattern for the rest? Maybe even _2f_ so it's slightly easier to read.

Also, it'd be nice to have some more tests for the new chars.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've improved the logic to make the output simpler for / and .. Does this look better?

Copy link
Copy Markdown
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.

Nice! A lot better to me.

@sapphi-red sapphi-red merged commit 3f24533 into vitejs:main Apr 20, 2026
16 checks passed
@sapphi-red sapphi-red deleted the fix/optimize-deps-handle-more-chars-that-will-be-sanitized branch April 20, 2026 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat: deps optimizer Esbuild Dependencies Optimization p2-edge-case Bug, but has workaround or limited in scope (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Vite 8] Error when importing package with $ symbol

3 participants