Normalize absolute paths on Windows #14187
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50852/ |
nicolo-ribaudo
added
the
PR: Bug Fix 🐛
| @@ -34,5 +34,5 @@ function resolveAbsoluteRuntime(moduleName: string, dirname: string) { | |||
| } | |||
|
|
|||
| export function resolveFSPath(path) { | |||
| return require.resolve(path); | |||
| return require.resolve(path).replace(/\\/g, "/"); | |||
There was a problem hiding this comment.
that might be somewhat too simplistic as the normalize-path package does far more than that:
https://github.com/jonschlinkert/normalize-path/blob/52c3a95ebebc2d98c1ad7606cbafa7e658656899/index.js
I would probably just use this package, but not sure how @nicolo-ribaudo and the rest of the team feel about adding dependencies
There was a problem hiding this comment.
Does require.resolve on windows return paths with the disk prefix (e.g. c:\path\to\resolved\file rather than \path\to\resolved\file)? If it doesn't, then this simple .replace is enough.
There was a problem hiding this comment.
require.resolve(path);
evaluates to
'C:\\Projects\\sources\\rollup-babel-bug\\node_modules\\@babel\\runtime\\helpers\\esm\\inherits.js'
So - remedy?
There was a problem hiding this comment.
Ok, does require("C:/Projects/sources/rollup-babel-bug/node_modules/@babel/runtime/helpers/inherits.js") work on windows? If it does, then the simple .replace is still ok. We don't need the full normalization algorithm here, because require.resolve always returns paths in a consistent shape.
There was a problem hiding this comment.
Well, https://github.com/atti187/rollup-babel-bug starts working on Windows with just this fix, but I'm not sure where else this path might be used? The preflightCheck might not be all of the story?
There was a problem hiding this comment.
And
typeof require("C:/Projects/sources/rollup-babel-bug/node_modules/@babel/runtime/helpers/inherits.js") === 'function';
return true. So it seems that node resolves this correctly, as well.
Are we good to go then? I'll update the fixture for the windows test...
There was a problem hiding this comment.
Yeah, I think it's ok to just use .replace for now (we also use it in other places to handle windows paths), and we can upgrade to proper normalization if anyone will ever find out that we need to do so.
I already asked to our bot to update the fixture 😉
nicolo-ribaudo
changed the title
github-actions
bot
added
the
outdated
Normalized file path, which otherwise causes issues in @rollup/plugin-babel, see issue at rollup/plugins#1089 and PR (incl. discussions) at rollup/plugins#1090