Fix unplugin path resolution (#774) #786
Conversation
|
I tested this on Windows and Linux, and found However, |
integration/unplugin/src/index.ts
Outdated
| @@ -178,7 +178,7 @@ const civetUnplugin = createUnplugin((options: PluginOptions = {}) => { | |||
|
|
|||
| const relativeId = path.relative( | |||
| process.cwd(), | |||
| path.resolve(path.dirname(importer ?? ''), id) | |||
| path.join(path.dirname(importer ?? ''), id) | |||
There was a problem hiding this comment.
Actually, I wonder whether this is correct... I would have guessed:
- If the path is relative, use
path.joinwithimporter, as you have it here - If the path is absolute, use
path.joinwith the project'srootsetting (which probably also needs to bejoined withprocess.cwd()because it too can be relative).
This is somewhat confirmed because I find src/main.civet can successfully import /module.civet, but I don't think it should be able to; it should have to import /src/module.civet I believe.
There was a problem hiding this comment.
Oops yeah you're correct, I overlooked that case.
|
I am very sure that |
integration/unplugin/src/index.ts
Outdated
|
|
||
| const absolutePath = path.isAbsolute(id) | ||
| ? path.join(path.join(process.cwd(), viteConfig.root ?? ''), id) | ||
| : path.join(path.dirname(importer ?? ''), id); |
There was a problem hiding this comment.
I'm pretty sure the latter case should use path.resolve, not path.join.
I pushed a new commit that cleans up this code. Could you check it?
There was a problem hiding this comment.
The result should be the same with path.resolve and path.join as we're already handling the case where id is an absolute path which is the only case where resolve and join differ
There was a problem hiding this comment.
Rest of the changes look good 👍
This PR should fix #774 and also make #785 not required.
path.resolvewould not consider the parent path if the child path starts with/, which made paths like/src/main.civetbe considered as system-absolute path instead of project-absolute.path.joinshould take them into account always, and make this work.The only issue this would face is not supporting system-absolute paths for civet, though I'm not sure if that is a feature that is actually used that much