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

fix plugin-transform-react-constant-elements transform JSXFrament but not add JSXExpressionContainer #16582

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

keiseiTi
Copy link

@keiseiTi keiseiTi commented Jun 21, 2024

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

fix #16449,the specific repair content is shown in the following figure

image

@babel-bot
Copy link
Collaborator

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

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thank you! Would you like to update the PR to get to the other possible expected output (the one where the fragment itself is hoisted), since it seems to be better?

And if yes, then also add a test where only one element can be hoisted and not the whole fragment:

function Component({ x }) {
   return <>
     <a>{x}</a>
     <b />
   </>;
}

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: jsx labels Jun 21, 2024
@keiseiTi
Copy link
Author

Thank you! Would you like to update the PR to get to the other possible expected output (the one where the fragment itself is hoisted), since it seems to be better?

And if yes, then also add a test where only one element can be hoisted and not the whole fragment:

function Component({ x }) {
   return <>
     <a>{x}</a>
     <b />
   </>;
}

sorry , I can't seem to understand what you mean . or want to add a test case ? or tell me your expected conversion result , then I will do that .

@JLHwung
Copy link
Contributor

JLHwung commented Jul 3, 2024

@keiseiTi Sorry for the late reply.

For the test case

function F() {
  return <><div>f</div></>;
}

The current output in this PR is

var _div;
function F() {
  return <>{_div || (_div = <div>f</div>)}</>;
}

This is already working but not optimal, since we still create a new JSX fragment <>{_div...}</> whenever F is invoked. Can you try to further optimize it to

var _frag;
function F() {
  return _frag || _frag = <><div>f</div></>;
}

And when you have done the optimization part, can you also add the following test case?

  • jsx-dynamic-children-in-fragment
function Component({ x }) {
   return <>
     <a>{x}</a>
     <b />
   </>;
}

The expected output should be

var _b;
function Component({
  x
}) {
  return <>
    <a>{x}</a>
    {_b || (_b = <b />)}
  </>;
}

This test is to verify that Babel can still hoist constant elements, e.g. <b /> within the fragment, when the fragment as a whole, should not be hoisted as x is dynamic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: jsx PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
4 participants