fix(vite-node): normalize relative paths to be from current working directory #952
Conversation
|
✔️ Deploy Preview for vitest-dev ready! 🔨 Explore the source changes: 2c4ff7c 🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/623201d1db036900096ba7da 😎 Browse the preview: https://deploy-preview-952--vitest-dev.netlify.app |
kalvenschraut
changed the title
antfu
changed the title
packages/vite-node/src/client.ts
Outdated
| @@ -56,9 +56,11 @@ export class ViteNodeRunner { | |||
| // probably means it was passed as variable | |||
| // and wasn't transformed by Vite | |||
| if (this.shouldResolveId(dep)) { | |||
| if (extname(dep)) dep = normalize(`${dirname(id)}/${dep}`) | |||
There was a problem hiding this comment.
So after digging up I know why I didn't like this code from the first glance.
id is treated as /src/path instead of ${root}/src/path - this is why resolveId doesn't work. we should combine root and id when needed and pass it to this.options.resolveId. or better teach resolveId how to handle imports without root in them (resolveId knows root from this.server.options.root)
And extname has nothing to do with it
So, in the end it is importer that is wrong, not the dep. This code in server.ts worked for me:
if (importer && !importer.startsWith(this.server.config.root))
importer = join(this.server.config.root, importer)Another path is to fix resolveId in Vite inself
There was a problem hiding this comment.
ID looked like the path from the current directory to the file that has the import unless I miss understood something. I needed to check extname otherwise I was hitting problems with node internals and package names, but I guess shouldResolveId is handling those cases now?
Regardless I was just trying fix one of the few problems remaining so I could fully adopt this library for my companies code base, any suggested fix for my problem will be fine for me since you know the code base better. I like trying to provide a solution rather then making another issue. I can add the above to the server.ts code in my PR.
There was a problem hiding this comment.
Try to add this to server.ts in your PR, yeah. The dep value is fine - since it is not absolute, it will be resolved according to importer, but importer was incorrect. So the solution to your problem would be to fix importer.
You also can import files without specifying the extension name, so the problem can rise again. I think the suggested solution is more well around.
There was a problem hiding this comment.
I updated my PR, I think I added it in the right spot. Test is still passing so seems to have worked.
If an imported file in a different directory is doing a dynamic import to a relative path that import is failing due to the relative import seeming to be from where we are running the tests from and not from where the file is imported from. I fixed this by just converting all relative imports with an extension to an absolute import. I don't see any issue with this, but I don't know the whole scope of this project. All tests are passing and this fixed my issue for my use case.