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 Babel issues in tests by applying the right transforms #1179

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests are failing after upgrade to 0.8.1
6 participants