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

Use named imports for babel types #13685

Merged
merged 4 commits into from Aug 18, 2021

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Aug 17, 2021

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

This PR is a follow-up to #13593 (review). In this PR we transform @babel/types namespace imports (e.g. import * as t from "@babel/types" to named imports (e.g. import { isIdentifier } from "@babel/types"). The named imports allow us to perform further optimization when transforming to commonjs modules.

Example

For example, before this PR, the named imports

import { isIdentifier } from "@babel/types";
isIdentifier(node)

are transformed as

const babel$types = require("@babel/types");
babel$types.isIdentifier(node)

After this PR, the named imports are transformed as

const _t = require("@babel/types");
const { isIdentifier } = _t
isIdentifier(node)

In #13593 we have identified a bottleneck of generator performance, accessing methods from @babel/types namespace is very slow. The optimized output now accesses namespace only once (per module).

Note that we can not generalize the optimization to any imports because ES imports are live bindings, which means if a module changed its exports on-the-fly, the updated values will be synchronized to the consumers. However, in the transpiled code, we takes a snapshot of these imports once and they won't be updated since then.

This approach is safe for most @babel/types usage in our current codebase since we don't modify the types exports, except in babel-traverse/src/path/index.ts where @babel/traverse is modifying the exports of @babel/types. Such usage is rare and we should consider if we should move on to a new approach.

The first commit is produced by a custom codemod.

Comparison to plugin-only approach

My first draft is a plugin transforming namespace import * as t to destructuring. Compared to current approach, the first approach has the following constraints:

  • Hard to determine which packages should be published because changes are only in build settings.
  • A valid method usage like const addComment = t.addComment becomes invalid after built, though we could avoid naming conflicts by sacrificing code readability.

Limitations

This PR does not change the indirect @babel/types usage such as

  • Use @babel/types from @babel/core
import { types as t } from "@babel/core"
  • Use @babel/types from the callback parameters of a plugin
module.exports = function({ types: t }) {
  
}

I think they should be updated, too. I will address them in separate PRs.

Further thoughts

Should we come up with a parameterized (🤷) assumption for commonjs transform?

{
  noImportLiveBindings: ["@babel/types"]
}

@JLHwung JLHwung added the PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories label Aug 17, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Aug 17, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48210/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 17, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d9c6681:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

👍 codemod approach makes the most sense and separating out follow up PRs, nice work!

@JLHwung JLHwung merged commit 614b486 into babel:main Aug 18, 2021
@JLHwung JLHwung deleted the use-named-import-for-babel-types branch August 18, 2021 14:28
@lightmare
Copy link
Contributor

lightmare commented Aug 18, 2021

I'm curious: is the slowdown due to the "emulation" of ES exports via getters? If that's the case, perhaps the issue could be alleviated at the source, i.e. in @babel/types by exporting fixed values instead.

Here's an idea: lightmare@5842734
It makes babel-helper-module-transforms act as if constantReexports = true when the source contains custom directive "@babel noLiveExports". Putting that directive into babel-types/src/index.ts then re-exports values, rather than getters.
What I haven't figured out yet, is how to remove the directive when exports are not being transpiled into requires.

@JLHwung
Copy link
Contributor Author

JLHwung commented Aug 18, 2021

is the slowdown due to the "emulation" of ES exports via getters?

That makes sense to me.

If that's the case, perhaps the issue could be alleviated at the source, i.e. in @babel/types by exporting fixed values instead.

Changing the exported values may break use cases such to manipulate the @babel/types exports and then to reuse it later. We can only do it on the consumer side when we are sure we don't need the live bindings. The good news is that when Babel 8 is published in ESM, it is likely we don't need this plugin, unless the native ESM live bindings have bad performance.

@lightmare
Copy link
Contributor

Changing the exported values may break use cases such to manipulate the @babel/types exports and then to reuse it later.

Not sure what you mean. Exports defined with Object.defineProperty(exports, key, { enumerable: true, get() { ... } } ) cannot be manipulated:

  • re-assignment fails silently
  • re-defineProperty fails loudly

Although that wasn't my intent, changing the exports to plain assignment (hence configurable: true) makes that kind of manipulation possible. I don't think doing that would be a good idea, though. Because all the other functions that @babel/types is re-exporting from submodules will still be referencing the original functions — individual submodules are importing directly from other submodules.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants