Visitar URL original
fix(new-url): decode percent-encoded paths in `new URL()` by theRizwan · Pull Request #11419 · rolldown/rolldown · GitHub
Skip to content

fix(new-url): decode percent-encoded paths in new URL() - #11419

Open
theRizwan wants to merge 1 commit into
rolldown:mainfrom
theRizwan:fix/new-url-percent-encoded
Open

theRizwan wants to merge 1 commit into
rolldown:mainfrom
theRizwan:fix/new-url-percent-encoded

Conversation

@theRizwan

Copy link
Copy Markdown
Contributor

Fixes case 2 of #11325. Case 1 is #11418, case 3 is a separate PR.

Problem

new URL() treats its first argument as a URL, so at runtime new URL("./my%20file.txt", import.meta.url) refers to my file.txt. With experimental.resolveNewUrlToAsset, rolldown passed the escaped text to the resolver and failed:

[UNRESOLVED_IMPORT] Could not resolve './my%20file.txt'

Fix

The scanner now decodes the path before adding the import record, using a new rolldown_utils::url::decode_uri_path that follows decodeURI:

  • only the path is decoded, the ?query and #fragment are kept as written;
  • escapes of reserved characters (%2F, %3F, %23 and the rest of decodeURI's reserved set) stay escaped, so %2F cannot turn into a path separator;
  • invalid escapes such as %zz are kept, and if the decoded bytes are not valid UTF-8 the specifier is used unchanged.

The rewritten output points at the emitted asset as before.

One behavior change worth flagging: a file whose name literally contains %20 (for example a%20b.txt) and is referenced as ./a%20b.txt was resolved before and is not now. That matches what the browser would request at runtime without bundling, so I think it is the right reading, but say if you would rather fall back to the raw path when the decoded one does not resolve.

Tests

  • New fixture topics/new_url/percent_encoded_path with ./my%20file.txt and ./%E2%9C%93.txt. It fails on main with UNRESOLVED_IMPORT for both.
  • Unit tests for decode_uri_path in rolldown_utils.

cargo test -p rolldown passes except test262_module_code, which fails the same way on main locally because the test262 submodule is not checked out. cargo test -p rolldown_utils, cargo fmt --check and cargo clippy are clean.

AI disclosure: I used Claude Code to help trace the code path and write the change and tests. I reviewed the change and ran the tests above myself.

`new URL()` treats its first argument as a URL, so `./my%20file.txt` refers
to `my file.txt` at runtime, but rolldown resolved the escaped text and
failed with UNRESOLVED_IMPORT. Decode the path the way `decodeURI` does
before resolving it, leaving the query, the fragment and escapes of reserved
characters as written.

Refs rolldown#11325
@codspeed

codspeed Bot commented Nov 1, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 4 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing theRizwan:fix/new-url-percent-encoded (81842d1) with main (d87a31e)

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@hyfdev hyfdev self-assigned this Nov 2, 2026
@hyfdev

hyfdev commented Nov 2, 2026

Copy link
Copy Markdown
Member

u8::from_str_radix accepts a leading +, so decode_uri_path("./a%+5.txt") returns ./a\u{5}.txt, while decodeURI rejects %+5. Could this check that both characters are hex digits, or use percent-encoding, which the workspace already depends on?

@hyfdev

hyfdev commented Nov 2, 2026

Copy link
Copy Markdown
Member

%5C decodes to \, which is a path separator on Windows, so the guarantee the description gives for %2F doesn't hold there. Node's fileURLToPath rejects both on Windows, and so does

if pathname.contains("%2F") || pathname.contains("%5C") {
; could %5C stay encoded too?

hyfdev added a commit that referenced this pull request Nov 3, 2026
…11420)

Fixes case 3 of #11325. Case 1 is #11418 and case 2 is #11419; this one
is independent of both.

## Problem

With `experimental.resolveNewUrlToAsset`, the query and fragment were
dropped when `new URL()` was rewritten:

```js
new URL("./sprite.svg?v=1", import.meta.url)  // -> new URL("assets/sprite-[hash].svg", import.meta.url)
new URL("./sprite.svg#icon", import.meta.url) // -> new URL("assets/sprite-[hash].svg", import.meta.url)
```

so `url.search` and `url.hash` were lost, which breaks uses like SVG
sprite fragments.

## Fix

In the `new URL()` finalizer, after computing the asset path, append the
part of the original specifier that follows the path (everything from
the first `?` or `#`, using `clean_url`). This matches Vite's
`vite:asset-import-meta-url`, which keeps the postfix when it rewrites
the URL. Resolution and the emitted asset are unchanged: all of `?v=1`,
`#icon`, `?v=1#icon` and the plain path still share one emitted file.

## Tests

New fixture `topics/new_url/keep_query_and_hash` covering a query, a
fragment, both, and the plain path. With snapshot updates disabled
(`INSTA_UPDATE=no CI=true`) it fails on `main` and passes here. No
existing snapshot changed.

`cargo test -p rolldown` passes except `test262_module_code`, which
fails the same way on `main` locally because the `test262` submodule is
not checked out. `cargo fmt --check` and `cargo clippy -p rolldown
--all-targets` are clean.

AI disclosure: I used Claude Code to help trace the code path and write
the change and test. I reviewed the change and ran the tests above
myself.

Co-authored-by: Yunfei He <i.heyunfei@gmail.com>

This branch has not been deployed

No deployments
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.

2 participants