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

babel-transform-react-inline-elements broken since beta35 due to #6984 #7178

Closed
jorrit opened this Issue Jan 9, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@jorrit
Contributor

jorrit commented Jan 9, 2018

bug: when transpiling the following JSX code in beta35 the generated code contains untranspiled JSX.

This happens only when @babel/transform-react-inline-elements is present and I think #6984 broke it.

The code is based on kriasoft/react-starter-kit.

Input Code

import React from 'react';
import Layout from '../../components/Layout';
import Contact from './Contact';
import graphqlclient from '../../core/graphqlclient';

const title = 'Neem contact op';

async function action({ data, req }) {
  const { isloaded } = data;
  let { user } = data;

  if (!isloaded) {
    const newdata = (await graphqlclient(`
      query ContactPage {
        me { displayname }
      }`, null, req));
    user = newdata.me;
  }

  return {
    title,
    chunks: ['contact'],
    component: <Layout user={user}><Contact title={title} /></Layout>,
    data: { user, isloaded: true },
  };
}

export default action;

Babel/Babylon Configuration (.babelrc, package.json, cli command)

          presets: [
            [
              '@babel/preset-env',
              {
                targets: {
                  browsers: [">1%", "last 4 versions", "Firefox ESR", "not ie < 9"],
                  forceAllTransforms: true,
                },
                modules: false,
                useBuiltIns: false,
                debug: false,
              },
            ],
            '@babel/preset-stage-2',
            ['@babel/preset-react'],
          ],
          plugins: [
            '@babel/transform-react-constant-elements',
            '@babel/transform-react-inline-elements',
            'transform-react-remove-prop-types',
          ],

Expected Behavior

In beta34 this was the output:

var action =
/*#__PURE__*/
function () {
  var _ref2 = _asyncToGenerator(
  /*#__PURE__*/
  regeneratorRuntime.mark(function _callee(_ref) {
    var data, req, isloaded, user, newdata;
    return regeneratorRuntime.wrap(function _callee$(_context) {
      while (1) {
        switch (_context.prev = _context.next) {
          case 0:
            data = _ref.data, req = _ref.req;
            isloaded = data.isloaded;
            user = data.user;

            if (isloaded) {
              _context.next = 8;
              break;
            }

            _context.next = 6;
            return graphqlclient("\n      query ContactPage {\n        me { displayname }\n      }", null, req);

          case 6:
            newdata = _context.sent;
            user = newdata.me;

          case 8:
            return _context.abrupt("return", {
              title: title,
              chunks: ['contact'],
              component: _jsx(Layout, {
                user: user
              }, void 0, _ref3),
              data: {
                user: user,
                isloaded: true
              }
            });

          case 9:
          case "end":
            return _context.stop();
        }
      }
    }, _callee, this);
  }));

  return function action(_x) {
    return _ref2.apply(this, arguments);
  };
}();

var REACT_ELEMENT_TYPE;

function _jsx(type, props, key, children) { if (!REACT_ELEMENT_TYPE) { REACT_ELEMENT_TYPE = typeof Symbol === "function" && Symbol.for && Symbol.for("react.element") || 0xeac7; } var defaultProps = type && type.defaultProps; var childrenLength = arguments.length - 3; if (!props && childrenLength !== 0) { props = {}; } if (props && defaultProps) { for (var propName in defaultProps) { if (props[propName] === void 0) { props[propName] = defaultProps[propName]; } } } else if (!props) { props = defaultProps || {}; } if (childrenLength === 1) { props.children = children; } else if (childrenLength > 1) { var childArray = new Array(childrenLength); for (var i = 0; i < childrenLength; i++) { childArray[i] = arguments[i + 3]; } props.children = childArray; } return { $$typeof: REACT_ELEMENT_TYPE, type: type, key: key === undefined ? null : '' + key, ref: null, props: props, _owner: null }; }

function _asyncToGenerator(fn) { return function () { var self = this, args = arguments; return new Promise(function (resolve, reject) { var gen = fn.apply(self, args); function step(key, arg) { try { var info = gen[key](arg); var value = info.value; } catch (error) { reject(error); return; } if (info.done) { resolve(value); } else { Promise.resolve(value).then(_next, _throw); } } function _next(value) { step("next", value); } function _throw(err) { step("throw", err); } _next(); }); }; }

import React from 'react';
import Layout from '../../components/Layout';
import Contact from './Contact';
import graphqlclient from '../../core/graphqlclient';
var title = 'Neem contact op';

var _ref3 = _jsx(Contact, {
  title: title
});

export default action;

Current Behavior

In beta 35 this is the output:
Note that there is still JSX in this code.

var REACT_ELEMENT_TYPE;

function _jsx(type, props, key, children) { if (!REACT_ELEMENT_TYPE) { REACT_ELEMENT_TYPE = typeof Symbol === "function" && Symbol.for && Symbol.for("react.element") || 0xeac7; } var defaultProps = type && type.defaultProps; var childrenLength = arguments.length - 3; if (!props && childrenLength !== 0) { props = {}; } if (props && defaultProps) { for (var propName in defaultProps) { if (props[propName] === void 0) { props[propName] = defaultProps[propName]; } } } else if (!props) { props = defaultProps || {}; } if (childrenLength === 1) { props.children = children; } else if (childrenLength > 1) { var childArray = new Array(childrenLength); for (var i = 0; i < childrenLength; i++) { childArray[i] = arguments[i + 3]; } props.children = childArray; } return { $$typeof: REACT_ELEMENT_TYPE, type: type, key: key === undefined ? null : '' + key, ref: null, props: props, _owner: null }; }

function _asyncToGenerator(fn) { return function () { var self = this, args = arguments; return new Promise(function (resolve, reject) { var gen = fn.apply(self, args); function step(key, arg) { try { var info = gen[key](arg); var value = info.value; } catch (error) { reject(error); return; } if (info.done) { resolve(value); } else { Promise.resolve(value).then(_next, _throw); } } function _next(value) { step("next", value); } function _throw(err) { step("throw", err); } _next(); }); }; }

import React from 'react';
import Layout from '../../components/Layout';
import Contact from './Contact';
import graphqlclient from '../../core/graphqlclient';
var title = 'Neem contact op';

function action(_x) {
  return _action.apply(this, arguments);
}

var _ref2 = <Contact title={title} />;

function _action() {
  _action = _asyncToGenerator(
  /*#__PURE__*/
  regeneratorRuntime.mark(function _callee(_ref) {
    var data, req, isloaded, user, newdata;
    return regeneratorRuntime.wrap(function _callee$(_context) {
      while (1) {
        switch (_context.prev = _context.next) {
          case 0:
            data = _ref.data, req = _ref.req;
            isloaded = data.isloaded;
            user = data.user;

            if (isloaded) {
              _context.next = 8;
              break;
            }

            _context.next = 6;
            return graphqlclient("\n      query ContactPage {\n        me { displayname }\n      }", null, req);

          case 6:
            newdata = _context.sent;
            user = newdata.me;

          case 8:
            return _context.abrupt("return", {
              title: title,
              chunks: ['contact'],
              component: _jsx(Layout, {
                user: user
              }, void 0, _ref2),
              data: {
                user: user,
                isloaded: true
              }
            });

          case 9:
          case "end":
            return _context.stop();
        }
      }
    }, _callee, this);
  }));
  return _action.apply(this, arguments);
}

export default action;

I captured both outputs by console.log-ing code in transpile in babel-loader.

Possible Solution

No clue.

Context

I disabled babel-transform-react-inline-elements for now.

Your Environment

software version(s)
Babel 7.0.0-beta.35
Babylon
node 9.3.0
npm 5.6.0
Operating System Windows 10 1709
@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Jan 9, 2018

Collaborator

Hey @jorrit! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Jan 9, 2018

Hey @jorrit! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@Andarist Andarist added the i: bug label Jan 9, 2018

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 9, 2018

Member

Ive confirmed that this is broken on latest master and is somehow connected to the used async functions syntax - if I remove async/await keywords the output is as expected

Trimmed down repro:

const title = 'Neem contact op';

async function action() {
  return <Contact title={title} />;
}
Member

Andarist commented Jan 9, 2018

Ive confirmed that this is broken on latest master and is somehow connected to the used async functions syntax - if I remove async/await keywords the output is as expected

Trimmed down repro:

const title = 'Neem contact op';

async function action() {
  return <Contact title={title} />;
}

@Andarist Andarist self-assigned this Jan 9, 2018

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 9, 2018

Member

This regression was introduced by #6984 . It didn't change much in the codebase and I dont see at glance what could have caused this problem, any ideas @nicolo-ribaudo ? My blind guess is that this is some requeuing problem, but I cannot locate it.

Member

Andarist commented Jan 9, 2018

This regression was introduced by #6984 . It didn't change much in the codebase and I dont see at glance what could have caused this problem, any ideas @nicolo-ribaudo ? My blind guess is that this is some requeuing problem, but I cannot locate it.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Jan 9, 2018

Member

I have no idea, since replaceWith (which I used to insert the new function in the ast) should requeue the node. I can investigate later if you don't find the problem.

Member

nicolo-ribaudo commented Jan 9, 2018

I have no idea, since replaceWith (which I used to insert the new function in the ast) should requeue the node. I can investigate later if you don't find the problem.

@Imundy

This comment has been minimized.

Show comment
Hide comment
@Imundy

Imundy Jan 11, 2018

Looking into this and it seems like something is going wrong here

  if (isDeclaration) {
    path.replaceWith(container[0]);
    path.insertAfter(container[1]);
  }

For some reason, the FunctionDeclaration we get from the template and pass to insertAfter doesn't seem like it gets properly traversed again. The simplest "fix" is:

  if (isDeclaration) {
    path.replaceWith(container[1]);
    path.insertBefore(container[0]);
  }

which means the declaration does get traversed correctly. However that causes other problems with the named function then (for example we end up exporting the REF instead of NAME in some tests) . I'm looking more into why this would be happening in the first place when using insertAfter. If anyone has any ideas to help point me in the right direction let me know!

Imundy commented Jan 11, 2018

Looking into this and it seems like something is going wrong here

  if (isDeclaration) {
    path.replaceWith(container[0]);
    path.insertAfter(container[1]);
  }

For some reason, the FunctionDeclaration we get from the template and pass to insertAfter doesn't seem like it gets properly traversed again. The simplest "fix" is:

  if (isDeclaration) {
    path.replaceWith(container[1]);
    path.insertBefore(container[0]);
  }

which means the declaration does get traversed correctly. However that causes other problems with the named function then (for example we end up exporting the REF instead of NAME in some tests) . I'm looking more into why this would be happening in the first place when using insertAfter. If anyone has any ideas to help point me in the right direction let me know!

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Jan 11, 2018

Member

Gr8 findings! This seems like some hidden internal bug. Does this happen only for FunctionDeclarations? Can be reproduced for easier cases?

Member

Andarist commented Jan 11, 2018

Gr8 findings! This seems like some hidden internal bug. Does this happen only for FunctionDeclarations? Can be reproduced for easier cases?

@Imundy

This comment has been minimized.

Show comment
Hide comment
@Imundy

Imundy Jan 12, 2018

Not sure, I'm a little bit lost tbh :-). I'm trying to find a simpler case to repro possibly by just using the insertAfter tests directly. It looks like replaceWith calls requeue on the NodePath which ends up calling the same thing that insertBefore/After calls - context.maybeQueue. They really look like they're doing the same thing to my untrained eyes but clearly there's a different result as far as traversal goes.
The last thing that happens as far as I can tell in the failure case is that the JSX transform tries to hoist the variable and requeue it but it doesn't appear it gets revisited.

Imundy commented Jan 12, 2018

Not sure, I'm a little bit lost tbh :-). I'm trying to find a simpler case to repro possibly by just using the insertAfter tests directly. It looks like replaceWith calls requeue on the NodePath which ends up calling the same thing that insertBefore/After calls - context.maybeQueue. They really look like they're doing the same thing to my untrained eyes but clearly there's a different result as far as traversal goes.
The last thing that happens as far as I can tell in the failure case is that the JSX transform tries to hoist the variable and requeue it but it doesn't appear it gets revisited.

@lock lock bot added the outdated label May 3, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2018

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