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

Merge injected es6 imports #15368

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jan 25, 2023

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

In this PR we try to multiple ES6 module imports based on the imported source. We track the last injected import in import-injector and persist that memory throughout the program's transpiling lifecycle.

The current implementation might break if there are external imports manipulation, i.e. users are modifying imports using tools other than helper-module-imports, because the injected import may not be the last import of the program and we will change the semantics when the imports have side effect. We could solve this by a new linear search to get the last import, but I think such cases are rare and we can postpone such fix to the future.

@JLHwung JLHwung added the PR: Output optimization 🔬 A type of pull request used for our changelog categories label Jan 25, 2023
@babel-bot
Copy link
Collaborator

babel-bot commented Jan 25, 2023

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

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 25, 2023

Some cases we can't optimize yet:

import * as react from "react";
import { jsx as _jsx } from "react/jsx-runtime";
import { createElement as _createElement } from "react";
import { jsxs as _jsxs } from "react/jsx-runtime";

Maybe we can introduce a new assumption like option (isolatedModule?) to allow addNamed() to merge imports more aggressively: not only merging to the last injected imports, but also any injected imports. As a jsx transformer we know that most jsx-runtime will satisfy this assumption and we can leverage that. General minifiers like terser will not combine such imports.

@nicolo-ribaudo
Copy link
Member

This code:

import { A } from "a";
import { B } from "b";
import { C } from "a";

is always equivalent to

import { A, C } from "a";
import { B } from "b";

because dependencies gets evaluated in the order of the first import that references them.

What's the reason to not do the more aggressive optimization by default? Just because it's more performant to not track all the imports?

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 26, 2023

@nicolo-ribaudo You are right! And they are hoisted, now I am wondering why terser does not implement that optimization. If it is generally safe, I think it'd be implemented by a minifier rather than us.

@JLHwung JLHwung marked this pull request as draft January 26, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Output optimization 🔬 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

3 participants