-
-
Notifications
You must be signed in to change notification settings - Fork 477
fix: escape virtual module import paths with apostrophes #2143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,45 @@ | ||||||
| import fs from 'fs-extra'; | ||||||
| import { tmpdir } from 'node:os'; | ||||||
| import { join } from 'node:path'; | ||||||
| import { afterEach, describe, expect, it } from 'vitest'; | ||||||
| import { resolveVirtualModules } from '../resolveVirtualModules'; | ||||||
| import { fakeResolvedConfig } from '../../../../utils/testing/fake-objects'; | ||||||
|
|
||||||
| const tempDirs: string[] = []; | ||||||
|
|
||||||
| afterEach(async () => { | ||||||
| await Promise.all(tempDirs.splice(0).map((dir) => fs.remove(dir))); | ||||||
| }); | ||||||
|
|
||||||
| describe('resolveVirtualModules', () => { | ||||||
| it.each([ | ||||||
| `import definition from 'virtual:user-background-entrypoint';`, | ||||||
| `import definition from "virtual:user-background-entrypoint";`, | ||||||
| ])( | ||||||
| 'should escape input paths when template contains %s', | ||||||
| async (template) => { | ||||||
| const wxtModuleDir = await fs.mkdtemp(join(tmpdir(), 'wxt-test-')); | ||||||
| tempDirs.push(wxtModuleDir); | ||||||
|
|
||||||
| await fs.outputFile( | ||||||
| join(wxtModuleDir, 'dist/virtual/background-entrypoint.mjs'), | ||||||
| template, | ||||||
| ); | ||||||
|
|
||||||
| const plugin = resolveVirtualModules( | ||||||
| fakeResolvedConfig({ wxtModuleDir }), | ||||||
| ).find( | ||||||
| (plugin) => plugin.name === 'wxt:resolve-virtual-background-entrypoint', | ||||||
| ); | ||||||
|
|
||||||
| expect(plugin).toBeDefined(); | ||||||
|
|
||||||
| const inputPath = `/tmp/foo'bar/background.ts`; | ||||||
| const code = await plugin!.load!( | ||||||
| '\0virtual:wxt-background-entrypoint?' + inputPath, | ||||||
| ); | ||||||
|
|
||||||
| expect(code).toBe(`import definition from ${JSON.stringify(inputPath)};`); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should hardcode the expected path here so it's clear what's being escaped and so the test doesn't rely on the same API as the runtime code. Is this correct? Or should there be one more
Suggested change
When I run it in the browser, I just get > console.log(JSON.stringify(`/tmp/foo'bar/background.ts`))
"/tmp/foo'bar/background.ts"
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait 🤦 I see, the problem is just that the string was defined with single quotes, so having another one in it is invalid: '/tmp/foo'bar/background.ts'Can you also add an expected test for when a project path has a double quote
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||||||
| }, | ||||||
| ); | ||||||
| }); | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention that we're testing apostrophes specifically in this test.