Skip to content
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

Windows: Import normalization can incorrectly resolve as both in and out of root #2422

Closed
GrygrFlzr opened this issue Mar 7, 2021 · 2 comments · Fixed by #2435
Closed

Windows: Import normalization can incorrectly resolve as both in and out of root #2422

GrygrFlzr opened this issue Mar 7, 2021 · 2 comments · Fixed by #2435

Comments

@GrygrFlzr
Copy link
Member

Describe the bug

On Windows, vite can incorrectly resolve the svelte runtime as both in and out of root, which causes two instances of the svelte runtime to be loaded. This issue does not seem to appear in Unix systems.

In the reproduction logs below, we can see:

In the browser's network tab:

http://localhost:3000/@fs/Users/GrygrFlzr/Documents/projects/vite-windows-imports/node_modules/svelte/internal/index.mjs?v=d0b9e871
http://localhost:3000/node_modules/svelte/internal/index.mjs?v=d0b9e871

The problem is likely in the import normalization:

// normalize all imports into resolved URLs
// e.g. `import 'foo'` -> `import '/@fs/.../node_modules/foo/index.js`
if (resolved.id.startsWith(root + '/')) {
// in root: infer short absolute path from root
url = resolved.id.slice(root.length)
} else if (fs.existsSync(cleanUrl(resolved.id))) {
// exists but out of root: rewrite to absolute /@fs/ paths
url = path.posix.join(FS_PREFIX + resolved.id)
} else {
url = resolved.id
}

Reproduction

Tested with npm, yarn, and pnpm - all reproduce the bug.
https://github.com/GrygrFlzr/vite-windows-imports

The reproduction makes use of svelte's setContext to better illustrate the bug causing two svelte runtimes to be loaded.

npm

npm i
npm run dev

Browser error:

Failed to init component
<App>
Error: Function called outside component initialization
    at get_current_component (http://localhost:3000/node_modules/svelte/internal/index.mjs?v=ad9e2cfc:649:15)
    at setContext (http://localhost:3000/node_modules/svelte/internal/index.mjs?v=ad9e2cfc:679:5)
    at instance (http://localhost:3000/src/App.svelte?import:69:2)
    at init (http://localhost:3000/@fs/Users/GrygrFlzr/Documents/projects/vite-windows-imports/node_modules/svelte/internal/index.mjs?v=ad9e2cfc:1474:11)
    at new App (http://localhost:3000/src/App.svelte?import:83:3)
    at createProxiedComponent (http://localhost:3000/node_modules/@svitejs/vite-plugin-svelte/node_modules/svelte-hmr/runtime/svelte-hooks.js:245:9)
    at new ProxyComponent (http://localhost:3000/node_modules/@svitejs/vite-plugin-svelte/node_modules/svelte-hmr/runtime/proxy.js:240:20)
    at new Proxy<App> (http://localhost:3000/node_modules/@svitejs/vite-plugin-svelte/node_modules/svelte-hmr/runtime/proxy.js:340:11)
    at http://localhost:3000/src/entry-client.js:3:13

System Info

  • vite version: 2.0.5
  • Operating System: Windows 10 10.0.19042
  • Node version: 14.15.3
  • Package managers: npm 7.5.2 / yarn 1.22.10 / pnpm 5.18.1
@ansbr
Copy link

ansbr commented Mar 7, 2021

Yes, I approved it, after updating svelte@next to newer version, it falls.

@GrygrFlzr
Copy link
Member Author

I've located the specific file and line which causes the stripping of C: from the path:

export function injectQuery(url: string, queryToInject: string) {
const { pathname, search, hash } = parseUrl(url)
return `${pathname}?${queryToInject}${search ? `&` + search.slice(1) : ''}${
hash || ''
}`
}

Line 122's usage of the deprecated url.parse method when used on C:/Users/GrygrFlzr/Documents/projects/vite-windows-imports/node_modules/.pnpm/svelte@3.35.0/node_modules/svelte/internal/index.mjs produces:

{
  protocol: 'c:',
  slashes: null,
  auth: null,
  host: '',
  port: null,
  hostname: '',
  hash: null,
  search: null,
  query: null,
  pathname: '/Users/GrygrFlzr/Documents/projects/vite-windows-imports/node_modules/.pnpm/svelte@3.35.0/node_modules/svelte/internal/index.mjs',
  path: '/Users/GrygrFlzr/Documents/projects/vite-windows-imports/node_modules/.pnpm/svelte@3.35.0/node_modules/svelte/internal/index.mjs',
  href: 'c:/Users/GrygrFlzr/Documents/projects/vite-windows-imports/node_modules/.pnpm/svelte@3.35.0/node_modules/svelte/internal/index.mjs'
}

Incorrectly assuming that c: is a protocol.

For the specific case of the minimal reproduction, it can be fixed using the recommended url.pathToFileURL...

-import { parse as parseUrl } from 'url'
+import { pathToFileURL } from 'url'

 // ...

 export function injectQuery(url: string, queryToInject: string) {
-  const { pathname, search, hash } = parseUrl(url)
+  const { pathname, search, hash } = pathToFileURL(url)
   return `${pathname}?${queryToInject}${search ? `&` + search.slice(1) : ''}${
     hash || ''
   }`
 }

...but this causes a lot of other tests to fail.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants