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

Alias from an absolute path to package name on Windows #17897

Closed
7 tasks done
Jinjiang opened this issue Aug 19, 2024 · 12 comments
Closed
7 tasks done

Alias from an absolute path to package name on Windows #17897

Jinjiang opened this issue Aug 19, 2024 · 12 comments

Comments

@Jinjiang
Copy link
Contributor

Jinjiang commented Aug 19, 2024

Describe the bug

The background is we are handling some entries or imports in absolute paths. Some of the paths are actually a package in node_modules. So we'd like to set alias to convert those absolute path imports into package imports.

It works perfectly on posix-based systems. However, we've found on Windows, some of the cases not working.

The following files are all CommonJS. So they won't work they couldn't be found or couldn't be recognized as a package to optimize.

  1. paths like /C:/xxx/node_modules/foo with { find: "/C:/xxx/node_modules/foo" } works
  2. paths like /@fs/C:/xxx/node_modules/foo with { find: "/@fs/C:/xxx/node_modules/foo" } works
  3. paths like C:\xxx\node_modules\foo with { find: "C:\xxx\node_modules\foo" } doesn't work
  4. paths like /@fs/C:\xxx\node_modules\foo with { find: "/@fs/C:\xxx\node_modules\foo" } doesn't work

Unfortunately, we have a lot existing imports in format 3. On Windows they are all failed to run.

Is there any chances we can support it?

Thanks.

Reproduction

https://github.com/Jinjiang/reproductions/tree/vite-windows-path-20240819

Steps to reproduce

# 0. it's only on Windows

# 1. install
pnpm install

# 2. edit the absolute paths in `main.mjs` and `vite.config.ts` according to your current directory

# 3. run
pnpm dev

Then you will find:

// it works
import './app.mjs?1';
// it works
import '/C:/Users/zhaoj/code/reproductions/app.mjs?2';
// it works
import '/@fs/C:/Users/zhaoj/code/reproductions/app.mjs?3';
// it works
import 'C:\\Users\\zhaoj\\code\\reproductions\\app.mjs?4';
// it works
import '/@fs/C:\\Users\\zhaoj\\code\\reproductions\\app.mjs?5';

// it works
import foo1 from 'foo/foo.cjs';

// it works
import foo2 from '/C:/Users/zhaoj/code/reproductions/node_modules/foo/foo.cjs';
// it works
import foo3 from '/@fs/C:/Users/zhaoj/code/reproductions/node_modules/foo/foo.cjs';

// it doesn't
import foo4 from 'C:\\Users\\zhaoj\\code\\reproductions\\node_modules\\foo\\foo.cjs';
// it doesn't
import foo5 from '/@fs/C:\\Users\\zhaoj\\code\\reproductions\\node_modules\\foo\\foo.cjs';

System Info

System:
    OS: Windows 11 10.0.26120
    CPU: (8) x64 Intel(R) Core(TM) i5-1035G7 CPU @ 1.20GHz
    Memory: 1.41 GB / 7.60 GB
  Binaries:
    Node: 20.11.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.14.3 - ~\AppData\Local\pnpm\pnpm.EXE
  Browsers:
    Edge: Chromium (127.0.2651.8)
    Internet Explorer: 11.0.26100.1

Used Package Manager

pnpm

Logs

Uncaught SyntaxError: The requested module '/node_modules/foo/foo.cjs?import&v=ede2c958' does not provide an export named 'default'

Validations

@sapphi-red
Copy link
Member

plugin-alias only matches ${keyname}/something and won't match ${keyname}\\something.
https://github.com/rollup/plugins/blob/8550c4b1925b246adbd3af48ed0e5f74f822c951/packages/alias/src/index.ts#L18

Even if we changed that to match \\, the replaced result would be ${keyname}\\foo.cjs. This is not a valid package specifier. It needs to be ${keyname}/foo.cjs or ${keyname}/foo or ${keyname}/dist/foo.cjs depending on the exports field.

I'm not sure if there's a way that would benefit generally.

@Jinjiang
Copy link
Contributor Author

I see. Thanks for the explanation.

I understand the complication, however, do you think it's possible to have a unified intermediate format inside Vite for alias matching or other id handling processes?

such like if { find: "/@fs/C:/xxx" } has a chance to match all the resources below:

  • /C:/xxx
  • /@fs/C:/xxx
  • C:\xxx
  • /@fs/C:\xxx

That would be very helpful in cases Vite accepts absolute paths on different OSs.

Thanks again.

@sapphi-red
Copy link
Member

Would you explain why you think this feature should be builtin? (as I guess it's possible by putting multiple alias entries and the usecase feels like an edge case)

Also why do you need to alias IDs containing /@fs? IIUC the ID resolution works like:

input id --(resolve 1)--> resolved id --(transform to url safe id)--> /@fs/foo --(resolve 2)--> resolved id

IDs with /@fs are already resolved by "resolve 1", so if you want to resolve it to a different path, I guess you can resolve it to a different path by "resolve 1" and not "resolve 2".

It would be helpful too if you can explain when each IDs happen as I don't know when it happens. Especially, the /C:/xxx case feels like a bug to me.

@Jinjiang
Copy link
Contributor Author

Jinjiang commented Aug 20, 2024

Sure.

In short the format /C:/xxx comes from dirname(new URL(import.meta.url).pathname).

To explain the whole original program on posix-based system first, some keypoints:

  • the entries (input of our program) are some custom absolute paths, like:

    /Users/zhaoj/code/reproductions/entry-foo.mjs
    
  • those entry files will import some absolute paths which are inside node_modules (and in CJS, which means they needs to be optimized as deps by Vite)

    // entry-foo.mjs
    import preview from '/Users/zhaoj/code/reproductions/node_modules/@teambit/preview/preview-entry.cjs'
    
  • those entry files also have some imports to a temporary package in node_modules/@teambit/_local, in this file, there are some ES imports of files from other packages and the specifiers are absolute paths

    // entry-foo.mjs
    import local from '/Users/zhaoj/code/reproductions/node_modules/@teambit/_local/local-entry.mjs'
    
    // @teambit/_local/local-entry.mjs
    import mounter from '/Users/zhaoj/code/reproductions/node_modules/.pnpm/xxx/mounter/mounter.cjs'
    
  • We set some aliases to make the absolute path imports work as package imports. e.g.:

    { find: '/Users/zhaoj/code/reproductions/node_modules/@teambit/preview/', replacement: '@teambit/preview/' }
    { find: '/Users/zhaoj/code/reproductions/node_modules/@teambit/_local/', replacement: '@teambit/_local/' }
    

So far it works very well on posix-based ones like macOS.

(I can simplify it into another reproduction if necessary.)

Then when things come to Windows, we've found in Vite dev server:

  1. C:\xxx didn't work as an entry, which will cause error "Not allowed to load local resource". Components preview not loading/working while bit start teambit/bit#9122

    Then we just blindly tried multiple other ways mentioned above and eventually found /C:/xxx worked as expected.

  2. Some CJS files in node_modules were not well-processed anymore. That's what I'd like ask originally in this issue.

  3. (another newly found one, didn't reproduce nor create an issue yet) the absolute path CJS imports in @teambit/_local didn't work neither. So far I've tried only relative paths work. e.g.

    // @teambit/_local/local-entry.mjs
    // it doesn't work (so far I only tried `/C:/xxx`)
    import mounter from '/C:/Users/zhaoj/code/reproductions/node_modules/.pnpm/xxx/mounter/mounter.cjs'
    // it works
    import mounter from '../../.pnpm/xxx/mounter/mounter.cjs'
    

That's all about it. I agree with you that /@fs/xxx might just be an internal thing. Just have no idea what the best practice should be in this case.

Thanks so much.

@sapphi-red
Copy link
Member

A reproduction would help me understand the problem.

In short the format /C:/xxx comes from dirname(new URL(import.meta.url).pathname).

This code is incorrect. It needs to be dirname(fileURLToPath(import.meta.url)). fileURLToPath has to be used here. (nodejs/node#37845)
Vite supports /C:/abspath, but that is composed from '/' + abspath.replaceAll('\\', '/') (I'm not sure if its support is intentional though).

@Jinjiang
Copy link
Contributor Author

I see. Back to the first issue, I tried dirname(new URL(import.meta.url).pathname) just because the output of fileURLToPath didn't work if it's the entry path which led to "Not allowed to load local resource".

e.g. <script src="C:\xxx\xxx.js"></script> in the index.html of a Vite project will get this error.

I understand according to the existing docs and issues dirname(new URL(import.meta.url).pathname) is incorrect. But which way could make it as least works?

And I will prepare another reproduction for the whole program later. 🙏

@sapphi-red
Copy link
Member

<script src="C:\xxx\xxx.js"></script> in the index.html of a Vite project will get this error.

I think this is a bug (made an issue: #17910)

@Jinjiang
Copy link
Contributor Author

@sapphi-red here is another reproduction for the minimized program:

https://github.com/Jinjiang/reproductions/tree/vite-windows-path-20240821

You can consider debug/init.mjs + debug/run.mjs is the original program, and debug/init-win.mjs + debug/run-win.mjs as how we tried to adapt Windows so far. 🙏

@sapphi-red
Copy link
Member

sapphi-red commented Aug 22, 2024

I took a look at your repro. This change worked for me.

diff --git a/debug/init.mjs b/debug/init.mjs
index 5c3c78b..bc49c26 100644
--- a/debug/init.mjs
+++ b/debug/init.mjs
@@ -1,8 +1,9 @@
 import { writeFileSync } from 'fs';
+import path from 'path';
 import { files } from './data.mjs';
 
-writeFileSync(files.index, `<script type="module" src="${files.main}"></script>`)
-writeFileSync(files.main, `import p from '${files.preview}'\nimport l from '${files.local}'\n\nconsole.log(p)\nconsole.log(l)`)
-writeFileSync(files.local, `import foo from '${files.foo}'\n\nconsole.log(foo)\n\nexport default 'local'`)
+writeFileSync(files.index, `<script type="module" src="${path.basename(files.main)}"></script>`) // workaround https://github.com/vitejs/vite/issues/17910
+writeFileSync(files.main, `import p from '${files.preview.replace(/\\/g, '/')}'\nimport l from '${files.local.replace(/\\/g, '/')}'\n\nconsole.log(p)\nconsole.log(l)`)
+writeFileSync(files.local, `import foo from '${files.foo.replace(/\\/g, '/')}'\n\nconsole.log(foo)\n\nexport default 'local'`)
 writeFileSync(files.preview, `module.exports = 'preview'`)
 writeFileSync(files.foo, `module.exports = 'foo'`)
diff --git a/debug/run.mjs b/debug/run.mjs
index c6c075e..08271a4 100644
--- a/debug/run.mjs
+++ b/debug/run.mjs
@@ -5,8 +5,8 @@ import { createServer } from 'vite'
 import { __dirname1, files } from './data.mjs';
 
 const alias = [
-  { find: dirname(files.preview), replacement: 'preview' },
-  { find: dirname(files.local), replacement: 'local' },
+  { find: dirname(files.preview).replace(/\\/g, '/'), replacement: 'preview' },
+  { find: dirname(files.local).replace(/\\/g, '/'), replacement: 'local' },
 ]
 
 // console.log(alias)

@Jinjiang
Copy link
Contributor Author

I see. It works. Thanks.

So may I understand the point is always converting C:\xxx to C:/xxx (plus the relative path for entries as walkaround)?

@sapphi-red
Copy link
Member

Yes. I guess you can make it work with \, but you need a bit more code to handle things described in #17897 (comment).

@Jinjiang
Copy link
Contributor Author

Understood. Thanks so much. Case closed to me.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants