Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, becauserequire.resolve
always returns paths in a consistent shape.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And
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.
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 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍