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

transform-react-constant-elements may hoist vars into unreachable code #6751

Open
Kovensky opened this Issue Nov 6, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@Kovensky
Member

Kovensky commented Nov 6, 2017

In certain circumstances involving a combination of locally scoped components and hoisted function declarations written inside unreachable code, the transform may write the hoisted jsx inside the unreachable code region.

I have placed the code in this report in astexplorer so the input/output can be played with (transform copied from master):

http://astexplorer.net/#/gist/a03cd893007f6c4ebb6a2bc7e56ef6f3/ce2d32e0aa49941e47fd1761971c76bedff81fc1

Input Code

function AComponent () {
  const CComponent = () => <div/>
  return <BComponent/>
    
  function BComponent () {
    return <CComponent/>
  }
}

This example is slightly contrived as the actual locally-scoped components in the RWC are function evaluation results.

Output Code

var _ref = <div/>;
function AComponent () {
  const CComponent = () => _ref
  return <BComponent/>

  var _ref2 = <CComponent/>;

  function BComponent () {
    return _ref2;
  }
}

Note how the _ref2 is declared after the return and, thus, is unreachable code. This will cause React to throw at runtime as BComponent returns undefined.

(This would rightly be a ReferenceError if the transform used let/const instead of var)


This may be somewhat low priority as actual components are never written with nested components inside their render functions; this case in RWC is a JSX factory function that just generates static JSX that is only rendered once.

@hzoo hzoo added the area: react label Nov 6, 2017

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Nov 7, 2017

Member

Given how what the transform does is just call path.hoist() on the JSX fragment that it detects as hoistable, this seems like a bug in @babel/traverse rather than a bug in transform-react-constant-elements.

Here is an astexplorer demonstrating this issue in a more contrived, but simpler example: http://astexplorer.net/#/gist/dce75ff8c622adafdd37786ea37db0c7/54e0ca43cf3f9dadfe6eccf0661ce5a4903314e6

Member

Kovensky commented Nov 7, 2017

Given how what the transform does is just call path.hoist() on the JSX fragment that it detects as hoistable, this seems like a bug in @babel/traverse rather than a bug in transform-react-constant-elements.

Here is an astexplorer demonstrating this issue in a more contrived, but simpler example: http://astexplorer.net/#/gist/dce75ff8c622adafdd37786ea37db0c7/54e0ca43cf3f9dadfe6eccf0661ce5a4903314e6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment