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.

Sign up for GitHub

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

Jump to bottom

Normalize absolute paths on Windows #14187

Merged
merged 2 commits into from Jan 22, 2022
Merged

Normalize absolute paths on Windows #14187

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9a0ca1
Normalized file path (#14185)
atti187 Jan 20, 2022
c6d038d
Update fixtures (Windows)
babel-bot Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/babel-plugin-transform-runtime/src/get-runtime-path/index.ts
View file
Open in desktop
Expand Up @@ -34,5 +34,5 @@ function resolveAbsoluteRuntime(moduleName: string, dirname: string) {
}

export function resolveFSPath(path) {
return require.resolve(path);
return require.resolve(path).replace(/\\/g, "/");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require.resolve(path);
evaluates to
'C:\\Projects\\sources\\rollup-babel-bug\\node_modules\\@babel\\runtime\\helpers\\esm\\inherits.js'

So - remedy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}