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

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

Merged
merged 4 commits into from
Feb 22, 2022
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a34837d
fix(babel): consider path delimeter on windows (#1089)
atti187 Jan 13, 2022
4626706
Fixed lint issues
atti187 Jan 14, 2022
a5f0758
Added reference to issue in babel
atti187 Jan 20, 2022
4431caf
chore: update comment linking to the resolved Babel issue
Andarist Feb 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/babel/src/preflightCheck.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ 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/;
// Revert to /\/helpers\/(esm\/)?inherits/ when Babel 8 gets released, this was fixed in https://github.com/babel/babel/issues/14185
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


export default async function preflightCheck(ctx, babelHelpers, transformOptions) {
const finalOptions = addBabelPlugin(transformOptions, helpersTestTransform);
Expand Down