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
Import the correct ./typeof.js
helper in @babel/runtime
#14081
Conversation
You can "test" this (i.e. prevent regressions) by unignoring |
* @param {*} node The string literal contains import path | ||
* // returns ast`"./setPrototypeOf"` | ||
* @example | ||
* adjustImportPath(ast`"@babel/runtime/helpers/typeof"`) |
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.
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"
.
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.
If there are injected imports for other helpers, wouldn't we want to transform them too?
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.
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.
72f4ad0
to
8244609
Compare
Thanks for the feedback, I updated the PR with your suggestions :) |
./typeof.js
helper in @babel/runtime
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.
Awesome, thanks!
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 ontypeof
:babel/packages/babel-helpers/src/helpers.ts
Lines 618 to 630 in 89cab43
When building
possibleConstructorReturn
(build-dist#247) thebabel-plugin-transform-runtime
plugin will run over it and inject the unimportedtypeof
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:
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 thebuild-dist
file is appreciated.Updated: Commited the helper generated code to avoid regressions.