fix(babel): consider path delimeter on windows (#1089) #1090
Conversation
|
Please correct the CI errors |
|
I have no idea why there are CI errors in your build pipeline... Look at the change and tell me it's because of it! |
|
Are you saying that because you don't have much experience with submitting pull requests that use Github actions to validate pull requests, or are you confused as to where the error is? If it's the latter, you can see how/why your changes caused CI to fail in two places:
As a practice, you should always look for a $ cd packages/babel
$ pnpm lint |
|
The latter - as running the linting on a Windows machine, hundreds (if not thousands) of errors showed up. I just assumed it was the same stuff showing up, and/or issues with the node images - have happened before. Anyway, I can take care of it tomorrow on my Ubuntu instance on WSL. |
|
Done! |
| @@ -34,7 +34,7 @@ function helpersTestTransform() { | |||
| const mismatchError = (actual, expected, filename) => | |||
| `You have declared using "${expected}" babelHelpers, but transforming ${filename} resulted in "${actual}". Please check your configuration.`; | |||
|
|
|||
| const inheritsHelperRe = /\/helpers\/(esm\/)?inherits/; | |||
| const inheritsHelperRe = /[\\/]+helpers[\\/]+(esm[\\/]+)?inherits/; | |||
There was a problem hiding this comment.
Hm, I'm rather surprised that this would be needed. I have never seen a windows path delimiter in the import specifier. Isn't this normalized by Babel and all the tools to use POSIX delimiter?
Do you have a repro case of the problem? I would love to take a look at that.
There was a problem hiding this comment.
Ok, I've found out the repro case in the linked ticket - thanks for that and for the detailed description of the problem there.
I think that the best thing to do would be to first try using normalize-path on this computed path:
https://github.com/facebook/create-react-app/blob/a422bf227cf5294a34d68696664e9568a152fd8f/packages/babel-preset-react-app/create.js#L51-L55
If a PR implementing such a change won't land in CRA cause their maintainers won't have time/bandwidth to handle this then we could consider proceeding with this PR here.
You can also try disabling the absoluteRuntime option in your babel-preset-react-app's options. I didn't have time to dig into why this is even the default etc but I think that for the vast majority of use cases this actually shouldn't be used.
There was a problem hiding this comment.
Ok - I'll create a PR for them and reference this issue, and we'll see where it takes us. I the meantime, I'll just skip the preflight checks.
There was a problem hiding this comment.
I dug into this a little bit more... seems like the windows-style import comes from https://github.com/babel/babel/blob/6ec66d8d2249e345a587d8d47722abd779be6416/packages/babel-plugin-transform-runtime/src/index.ts#L241. The absoluteRuntime config value isn't used in this computation, so changes in CRA has no effect.
So the question is then - should we try to land a change in babel-plugin-transform-runtime, or stick with the regex fix in this PR?
There was a problem hiding this comment.
So the question is then - should we try to land a change in babel-plugin-transform-runtime, or stick with the regex fix in this PR?
I'm leaning towards... both 😅 I would appreciate it if you could:
- prepare a PR to Babel
- add a comment in the code about this issue and link to the created PR
|
@shellscape I've approved the changes - feel free to land this |
Rollup Plugin Name:
{name}This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
What it says on the tin - fixes #1089