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

Import the correct ./typeof.js helper in @babel/runtime #14081

Merged

Conversation

exb
Copy link
Contributor

@exb exb commented Dec 28, 2021

Q                       A
Fixed Issues? Fixes #13786
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? No
Documentation PR Link
Any Dependency Changes?
License MIT

Context

When a helper depends on another helper which is not explicitly imported (see code snippet below), the babel-plugin-transform-runtime will inject the helper at the top but it will not inject it in the proper module format version.

Let's take the possibleConstructorReturn as an example. The helper itself does not "explicitly" depend on typeof:

helpers.possibleConstructorReturn = helper("7.0.0-beta.0")`
import assertThisInitialized from "assertThisInitialized";
export default function _possibleConstructorReturn(self, call) {
if (call && (typeof call === "object" || typeof call === "function")) {
return call;
} else if (call !== void 0) {
throw new TypeError("Derived constructors may only return object or undefined");
}
return assertThisInitialized(self);
}
`;

When building possibleConstructorReturn (build-dist#247) the babel-plugin-transform-runtime plugin will run over it and inject the unimported typeof at the very top as follows:

+ import _typeof from "@babel/runtime/helpers/typeof";
import assertThisInitialized from "./assertThisInitialized.js";

The issue is that the injected module format is CJS instead of being picked up by the context. The desired output should be the following:

- import _typeof from "@babel/runtime/helpers/typeof";
+ import _typeof from "./typeof.js";
import assertThisInitialized from "./assertThisInitialized.js";

The solution
To fix the issue I updated the adjustImportPath function to rewrite the imports generated from the babel-plugin-transform-runtime as relative path imports.

I realize that this PR is just patching the problem rather than making the transform runtime plugin import the proper module format in the first place. Doing that, however, would require us to do some rethinking of the build-dist file, which considering the issue was marked as "good first issue", I deemed it was out of scope. Let me know if I should proceed otherwise.

Testing
Guidance on testing the build-dist file is appreciated.
Updated: Commited the helper generated code to avoid regressions.

@JLHwung JLHwung self-requested a review Dec 28, 2021
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 28, 2021

You can "test" this (i.e. prevent regressions) by unignoring possibleConstructorReturn.js from .gitignore, and commit the helper generated code. We already do that, for example, with toArray.

* @param {*} node The string literal contains import path
* // returns ast`"./setPrototypeOf"`
* @example
* adjustImportPath(ast`"@babel/runtime/helpers/typeof"`)
Copy link
Contributor

@JLHwung JLHwung Dec 28, 2021

Choose a reason for hiding this comment

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

The import "@babel/runtime/helpers/typeof" is generated by @babel/plugin-transform-typeof-symbol which runs before the buildRuntimeRewritePlugin plugin. In this case I will prefer just convert "@babel/runtime/helpers/typeof" to "./typeof": So this plugin does not accidentally support "@babel/runtime/helpers/another-helper".

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 28, 2021

Choose a reason for hiding this comment

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

If there are injected imports for other helpers, wouldn't we want to transform them too?

Copy link
Contributor

@JLHwung JLHwung Dec 29, 2021

Choose a reason for hiding this comment

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

Yes they should.

Not related to this PR: I realized that the generated helper will target to browserlists's default in Babel 8 since we didn't supply a target in the build-dist. I guess it's fine to bump the targets but we should convey that in the changelog.

@exb exb force-pushed the fix/import-correct-helper-module-version branch from 72f4ad0 to 8244609 Compare Dec 29, 2021
@exb
Copy link
Contributor Author

@exb exb commented Dec 30, 2021

Thanks for the feedback, I updated the PR with your suggestions :)

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 label Dec 30, 2021
@nicolo-ribaudo nicolo-ribaudo changed the title Import correct module format in helpers depending on other helpers Import the correct ./typeof.js helper in @babel/runtime Dec 30, 2021
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Awesome, thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit f69f1a8 into babel:main Dec 30, 2021
24 checks passed
@github-actions github-actions bot added the outdated label Apr 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants