fix: unescape webpack/rspack paths in load and transform by edemaine · Pull Request #592 · unjs/unplugin · GitHub
Skip to content

fix: unescape webpack/rspack paths in load and transform#592

Open
edemaine wants to merge 3 commits intounjs:mainfrom
edemaine:webpack-path-escape
Open

fix: unescape webpack/rspack paths in load and transform#592
edemaine wants to merge 3 commits intounjs:mainfrom
edemaine:webpack-path-escape

Conversation

@edemaine
Copy link
Copy Markdown
Contributor

@edemaine edemaine commented Mar 15, 2026

Resolves #591 by unescaping paths before passing into load and transform.

I'm open to better / less redundant ways to test. I wanted to test that all hooks unescape (or never escape), on all platforms.

Summary by CodeRabbit

  • Bug Fixes

    • Improved unescaping and normalization of resource paths so inputs with escaped or special characters (e.g., null-terminated sequences and zero‑width-space hashes) are handled correctly during load/transform flows.
  • Tests

    • Added end-to-end fixtures and tests validating hash-path behavior and plugin integration across Vite, Rollup, Webpack, esbuild, Rspack, Farm, and Bun.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 15, 2026

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Mar 15, 2026

Open in StackBlitz

npm i https://pkg.pr.new/unplugin@592

commit: 9014374

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
test/fixtures/hash-path/unplugin.js (1)

40-43: Optionally add explicit token-not-found guards for clearer test failures.

Right now overwrite(...) relies on implicit failure behavior when tokens are missing. A direct guard would make fixture failures more diagnosable.

♻️ Suggested small robustness tweak
       const s = new MagicString(code)
       const index = code.indexOf('msg#hash')
+      if (index === -1)
+        throw new Error(`load expected token "msg#hash" in ${JSON.stringify(id)}`)

       s.overwrite(index, index + 'msg#hash'.length, 'msg -> through the load hook -> __unplugin__#hash')
       return s.toString()
@@
       const s = new MagicString(code)
       const index = code.indexOf('__unplugin__')
+      if (index === -1)
+        throw new Error(`transform expected token "__unplugin__" in ${JSON.stringify(id)}`)

       s.overwrite(index, index + '__unplugin__'.length, 'transform')
       return {

Also applies to: 57-60

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/fixtures/hash-path/unplugin.js` around lines 40 - 43, The test fixture
silently assumes the token exists before calling s.overwrite; add explicit
guards that check the result of code.indexOf('msg#hash') (and similarly the
other occurrence around lines 57-60) and throw a clear error if indexOf returns
-1 so failures show a descriptive message; locate the token search using
code.indexOf('msg#hash') and the mutation via s.overwrite(...) (and the paired
s.toString() return) and validate the index before calling s.overwrite to
produce a meaningful test-failure message when the token is missing.
test/fixtures/hash-path/__test__/build.test.ts (1)

10-38: Optional: collapse repeated bundler assertions into a table-driven loop.

Current tests are solid; this is just a maintainability cleanup to reduce duplication and ease future fixture additions.

♻️ Proposed refactor
 describe('hash-path hooks', () => {
-  it('vite', async () => {
-    const content = await fs.readFile(r('vite/main.js.mjs'), 'utf-8')
-    expect(content).toContain(expected)
-  })
-
-  it('rollup', async () => {
-    const content = await fs.readFile(r('rollup/main.js'), 'utf-8')
-    expect(content).toContain(expected)
-  })
-
-  it('webpack', async () => {
-    const content = await fs.readFile(r('webpack/main.js'), 'utf-8')
-    expect(content).toContain(expected)
-  })
-
-  it('esbuild', async () => {
-    const content = await fs.readFile(r('esbuild/main.js'), 'utf-8')
-    expect(content).toContain(expected)
-  })
-
-  it('rspack', async () => {
-    const content = await fs.readFile(r('rspack/main.js'), 'utf-8')
-    expect(content).toContain(expected)
-  })
-
-  it('farm', async () => {
-    const content = await fs.readFile(r('farm/main.js'), 'utf-8')
-    expect(content).toContain(expected)
-  })
+  const cases: Array<[string, string]> = [
+    ['vite', 'vite/main.js.mjs'],
+    ['rollup', 'rollup/main.js'],
+    ['webpack', 'webpack/main.js'],
+    ['esbuild', 'esbuild/main.js'],
+    ['rspack', 'rspack/main.js'],
+    ['farm', 'farm/main.js'],
+  ]
+
+  for (const [name, file] of cases) {
+    it(name, async () => {
+      const content = await fs.readFile(r(file), 'utf-8')
+      expect(content).toContain(expected)
+    })
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/fixtures/hash-path/__test__/build.test.ts` around lines 10 - 38, Tests
repeat the same assertion for multiple bundlers; refactor by replacing the
repeated it(...) blocks with a table-driven loop: create an array of bundler
names (e.g., ['vite','rollup','webpack','esbuild','rspack','farm']), iterate
over it and for each name call it(bundleName, async () => { const content =
await fs.readFile(r(`${bundleName}/main.js${bundleName === 'vite' ? '.mjs' :
''}`), 'utf-8'); expect(content).toContain(expected); }); use the existing r
helper, fs.readFile, and expected variable so behavior is unchanged but
duplication is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/fixtures/hash-path/__test__/build.test.ts`:
- Around line 10-38: Tests repeat the same assertion for multiple bundlers;
refactor by replacing the repeated it(...) blocks with a table-driven loop:
create an array of bundler names (e.g.,
['vite','rollup','webpack','esbuild','rspack','farm']), iterate over it and for
each name call it(bundleName, async () => { const content = await
fs.readFile(r(`${bundleName}/main.js${bundleName === 'vite' ? '.mjs' : ''}`),
'utf-8'); expect(content).toContain(expected); }); use the existing r helper,
fs.readFile, and expected variable so behavior is unchanged but duplication is
removed.

In `@test/fixtures/hash-path/unplugin.js`:
- Around line 40-43: The test fixture silently assumes the token exists before
calling s.overwrite; add explicit guards that check the result of
code.indexOf('msg#hash') (and similarly the other occurrence around lines 57-60)
and throw a clear error if indexOf returns -1 so failures show a descriptive
message; locate the token search using code.indexOf('msg#hash') and the mutation
via s.overwrite(...) (and the paired s.toString() return) and validate the index
before calling s.overwrite to produce a meaningful test-failure message when the
token is missing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4df8609d-b47d-49b1-b362-eb9c98c3ae02

📥 Commits

Reviewing files that changed from the base of the PR and between 111a437 and 32b8256.

📒 Files selected for processing (17)
  • src/rspack/loaders/load.ts
  • src/rspack/loaders/transform.ts
  • src/utils/webpack-like.ts
  • src/webpack/loaders/load.ts
  • src/webpack/loaders/transform.ts
  • test/fixtures/hash-path/__test__/build.test.ts
  • test/fixtures/hash-path/bun.config.js
  • test/fixtures/hash-path/esbuild.config.js
  • test/fixtures/hash-path/farm.config.js
  • test/fixtures/hash-path/rollup.config.js
  • test/fixtures/hash-path/rspack.config.js
  • test/fixtures/hash-path/src/main.js
  • test/fixtures/hash-path/src/msg#hash.js
  • test/fixtures/hash-path/unplugin.js
  • test/fixtures/hash-path/vite.config.js
  • test/fixtures/hash-path/webpack.config.js
  • test/fixtures/load/unplugin.js

@edemaine edemaine changed the title Unescape webpack/rspack paths in load and transform fix: unescape webpack/rspack paths in load and transform Mar 15, 2026
@antfu
Copy link
Copy Markdown
Member

antfu commented Mar 16, 2026

Tests are failing, would you also check them? Thanks

@edemaine
Copy link
Copy Markdown
Contributor Author

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.

Webpack/rspack call load and transform with escaped paths

2 participants