Skip to content

Fix Babel issues in tests by applying the right transforms #1179

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 6, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Dec 6, 2016

This is a followup to #1177 with a less aggressive fix.
For context, read threads #1156 and #1160.

Why this works:

  • Babel has a bug where object/spread depends on parameters transform: Object-rest-spread doesn't work in case of destructuring with default object parameter babel/babel#4851. So we have to enable parameters.
  • Including regenerator transform in test environment was incorrect. Its async: false option only made sense for development and production configuration where we use babel-preset-latest, and so async functions are already being handled. But for test environment we use babel-preset-env instead of babel-preset-latest, and so including regenerator second time is redundant (and somehow breaks things—@hzoo could you clarify why this happened?)

I am remove destructuring and arrow-functions because they were added as stopgap measure in #1177 but turned out unnecessary per #1156 (comment).

Fixes #1156 and #1160.

@gaearon gaearon added this to the 0.8.2 milestone Dec 6, 2016
@gaearon
Copy link
Contributor Author

gaearon commented Dec 6, 2016

Test file I used in different environments:

import React, { Component } from 'react';
import logo from './logo.svg';
import './App.css';

export function *foo() {
  yield 42;
  yield 49;
}

export async function lol() {
  return await Promise.resolve(5000);
}

class App extends Component {
  render() {
    return (
      <div className="App">
        <div className="App-header">
          <img src={logo} className="App-logo" alt="logo" />
          <h2>Welcome to React</h2>
        </div>
        <p className="App-intro">
          To get started, edit <code>src/App.js</code> and save to reload.
        </p>
      </div>
    );
  }
}

var addIndex = function addIndex({ index, ...routes } = {}, base) {
};
function doSometing() {
  return {};
}

const getId = async () => {
  const { id } = await doSometing(42)
  return id
};

const getId2 = async () => {
  const p = await doSometing(42)
  const { id } = p
  return id
};

function Record() {
  return function() {}
}

const User = new Record({ id: NaN })
new User({ id: 42 })

export const x = async () => {
  var y = foo()
  var z = y.next()
  while (!z.done) {
    console.log(z.value);
    z = y.next();
  }
  const wow = await lol();
  console.log(wow)
};
x();

export default App;

I tried Chrome, IE11, Node 4, and Node 7.

@gaearon gaearon merged commit d8dfdb0 into master Dec 6, 2016
@gaearon gaearon deleted the fix-babel branch December 6, 2016 15:10
@hzoo
Copy link

hzoo commented Dec 6, 2016

Just more explaination for future ref

Babel has a bug where object/spread depends on parameters transform: babel/babel#4851. So we have to enable parameters.

This is simply because I didn't account for a default in object rest spread so currently the object/rest spread transform doesn't do anything to the code and thus the syntax error.

But for test environment we use babel-preset-env instead of babel-preset-latest, and so including regenerator second time is redundant (and somehow breaks things—@hzoo could you clarify why this happened?)

Still need to look into it

@hzoo
Copy link

hzoo commented Dec 6, 2016

Expected type "Expression" with option {} is coming from babel-types and after getting the stack trace from jest it is coming from transform-regenerator/lib/emit.js

Error: /cra-jest-babel-types/src/merchants.js: ObjectPattern
    at Object.t.(anonymous function) [as assertExpression] (/cra-jest-babel-types/node_modules/babel-types/lib/index.js:362:13)
    at finish (/cra-jest-babel-types/node_modules/babel-plugin-transform-regenerator/lib/emit.js:623:7)
    at Emitter.Ep.explodeExpression (/cra-jest-babel-types/node_modules/babel-plugin-transform-regenerator/lib/emit.js:632:12)
Ep.explodeExpression = function (path, ignoreResult) {
  var expr = path.node;
  if (expr) {
    t.assertExpression(expr);

which is checking an Expression but getting an ObjectPattern via const {result} = doSometing(); (which isn't aliased as an expression since it isn't)

@benjamn just reimplemented the transform in facebook/regenerator#259
and has a PR to update it with some other fixes via babel/babel#4881 so maybe that fixes it, not sure.

@benjamn
Copy link

benjamn commented Dec 6, 2016

Other transforms are sometimes necessary to satisfy the preconditions of Regenerator. That's why regenerator-preset exists. This PR seems like the right approach, though the errors could always be more helpful.

EnoahNetzach pushed a commit to EnoahNetzach/create-react-app that referenced this pull request Dec 6, 2016
@gaearon
Copy link
Contributor Author

gaearon commented Dec 6, 2016

Do we need to add everything from regenerator-preset in the mode when we use -env?

@benjamn
Copy link

benjamn commented Dec 6, 2016

I wouldn't literally use regenerator-preset just yet, since babel/babel#4881 still needs to be merged, but the other plugins are probably worth including whenever Regenerator is used.

@gaearon
Copy link
Contributor Author

gaearon commented Dec 6, 2016

Hmm. Is there ever a case where Node supports classes/arrows/block-scoping/for-of but not generators?

@Jessidhia
Copy link

Jessidhia commented Dec 7, 2016

Node 4 / 5 don't support generator return / throw, though they support everything else about them. This also comes into play when break/return/throwing inside a for..of loop, but for..of doesn't implement that either in Node < 6; so you could argue Node 4 / 5 don't actually support for-of. io.js, which is the closest Node 4 predecessor, doesn't support generators but they also don't support arrows / block-scoping.

@gaearon
Copy link
Contributor Author

gaearon commented Dec 7, 2016

@hzoo Does -env take care of this? Should I file an issue?

@Jessidhia
Copy link

Jessidhia commented Dec 7, 2016

@gaearon Looks like it does, for both for-of and regenerator; generators were fixed a v8 release before for-of (according to compat-table).

@gaearon
Copy link
Contributor Author

gaearon commented Dec 7, 2016

Sounds great. Thanks!

mofelee added a commit to xiaohu-developer/create-react-app that referenced this pull request Dec 7, 2016
* master: (30 commits)
  Relax peerDependencies for ESLint preset (facebook#1191)
  Update Webpack to fix source map issues (facebook#1188)
  Update webpack prod config (facebook#1181)
  Chrome 'open tab' reuse an empty tab when possible (facebook#1165)
  Use file-loader for svgs (facebook#1180)
  Fix Babel issues in tests by applying the right transforms (facebook#1179)
  [babel-preset-react-app] Temporary fix missing babel plugins (facebook#1177)
  Add Subresource Integrity support (facebook#1176)
  Remove path module from webpack config on eject. (facebook#1175)
  Don't strip stack traces of evaluated webpack bundles (facebook#1050)
  Add deploy to Firebase CDN on template's README (Closes facebook#374) (facebook#1143)
  Update e2e.sh (facebook#1167)
  Document what npm build does and pushState (facebook#933)
  Fix minor typo/grammar (facebook#1099)
  Add "npm run build silently fails" to Troubleshooting (facebook#1168)
  Add testURL to jest config (facebook#1120)
  Make jsx-no-undef rule an error (facebook#1159)
  Update CHANGELOG.md
  Publish
  Update changelog for 0.8.1
  ...
@gaearon
Copy link
Contributor Author

gaearon commented Dec 7, 2016

Fixed in 0.8.2.
Please verify.

https://github.com/facebookincubator/create-react-app/releases/tag/v0.8.2

@EnoahNetzach
Copy link
Contributor

tests are OK for me

alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests are failing after upgrade to 0.8.1
6 participants