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.

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

fix(babel): consider path delimeter on windows (#1089) #1090

Merged
merged 4 commits into from Feb 22, 2022

Conversation

atti187
Copy link
Contributor

@atti187 atti187 commented Jan 13, 2022

Rollup Plugin Name: {name}

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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

@shellscape
Copy link
Collaborator

Please correct the CI errors

@atti187
Copy link
Contributor Author

atti187 commented Jan 13, 2022

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!

@shellscape
Copy link
Collaborator

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 lint and/or build in package.json for the project you're editing, and run either or both before submitting a pull request. This repo uses pnpm, so you should have run:

$ cd packages/babel
$ pnpm lint

@atti187
Copy link
Contributor Author

atti187 commented Jan 13, 2022

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.

@atti187
Copy link
Contributor Author

atti187 commented Jan 14, 2022

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/;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

@Andarist @atti187 checking in on this PR.

@Andarist
Copy link
Member

@shellscape I've approved the changes - feel free to land this

@shellscape shellscape merged commit c259de5 into rollup:master Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin-babel] Unexpected situation when using with babel-preset-react-app on Windows
3 participants