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

Remove unnecessary transform plugins for object spread to work #1052

Merged
merged 2 commits into from
Nov 17, 2016

Conversation

valscion
Copy link
Contributor

@valscion valscion commented Nov 17, 2016

The babel-plugin-transform-object-rest-spread v6.19.0 update made the other two plugins unnecessary as it contains the fixes done in babel/babel#4755

https://github.com/babel/babel/blob/v6.19.0/CHANGELOG.md

Ref #904 (comment) and #927

Test plan

1. Use node v6.7.0

If you use nvm, this is how:

nvm install v6.7.0
nvm use v6.7.0

2. (Optional, I think) Use npm v2

npm install -g npm@2.x

3. Modify App.test.js

Modify packages/react-scripts/template/src/App.test.js to look like this:

import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

it('renders without crashing', () => {
  const fn = ({ a, ...otherProps }) => otherProps;

  fn({ a: 1, b: 2 });

  console.log(fn({ a: 1, b: 2 }));

  const div = document.createElement('div');
  ReactDOM.render(<App />, div);
});

4. Install all the packages and run the test script

npm install
npm test

@gaearon
Copy link
Contributor

gaearon commented Nov 17, 2016

What does the failure look like?

@gaearon
Copy link
Contributor

gaearon commented Nov 17, 2016

cc @hzoo

valscion added a commit to valscion/create-react-app that referenced this pull request Nov 17, 2016
@valscion
Copy link
Contributor Author

What does the failure look like?

It looks like this:

FAIL  src/App.test.js
  ● Test suite failed to run

    /Users/vesa/code/muut/create-react-app/packages/react-scripts/template/src/App.test.js:6
      const fn = ({ a, ...otherProps }) => otherProps;
                       ^^^
    SyntaxError: Unexpected token ...

      at transformAndBuildScript (../node_modules/jest/node_modules/jest-cli/node_modules/jest-runtime/build/transform.js:284:10)
      at process._tickCallback (../../../internal/process/next_tick.js:103:7)

I'm not sure whether I have everything set up properly, but I've cleaned my node_modules multiple times across every package. I've followed the test plan many times, but still get the same error.

I added a commit valscion@9efbc7c that contains patch that should be legit, but crashes. This gist contains both the patch and the stacktrace: https://gist.github.com/valscion/25989e714380f2eabe52d170d6ca6f41

@hzoo
Copy link

hzoo commented Nov 17, 2016

babel-plugin-transform-class-properties

You should be updating object-rest-spread to v6.19.0 😄 that's all - it's currently "babel-plugin-transform-object-rest-spread": "6.16.0",

@valscion
Copy link
Contributor Author

D'oh!

The `babel-plugin-transform-object-rest-spread` v6.19.0 update will
allow us to remove the `babel-plugin-transform-es2015-destructuring` and
`babel-plugin-transform-es2015-parameters` as the object rest spread
transform will now work standalone and not require additional tranforms
The `babel-plugin-transform-object-rest-spread` v6.19.0 update makes
these plugins unnecessary, as v6.19.0 can be used stand-alone
@valscion valscion force-pushed the re-disable-babel-destructuring branch from 5cd0933 to 7554d30 Compare November 17, 2016 14:34
@valscion
Copy link
Contributor Author

Ok, that should fix it, and now I don't get any errors anymore 😅 😅 😅 😅 Thanks @hzoo for pointing out my silly mistake! Funny how some things slip your eye so easily 😂

@hzoo
Copy link

hzoo commented Nov 17, 2016

I would add the 2 tests back as well

const { a, ...z } = obj;
const fn = ({ a, ...otherProps }) => otherProps;

@valscion
Copy link
Contributor Author

I would add the 2 tests back as well

They haven't been there ever, I just used them to test the local changes. I think it's out of scope of this PR?

@gaearon gaearon added this to the 0.8.0 milestone Nov 17, 2016
@gaearon gaearon merged commit 4a7f78e into facebook:master Nov 17, 2016
@gaearon
Copy link
Contributor

gaearon commented Nov 17, 2016

LGTM

eXtreme added a commit to eXtreme/create-react-app that referenced this pull request Nov 18, 2016
* pull2:
  Support Yarn (facebook#898)
  Fix chrome tab reuse (facebook#1035)
  Remove unnecessary transform plugins for object spread to work (facebook#1052)
  Clears the usage of react-jsx-source & react-jsx-self (facebook#992)
  Update babel-present-env and use node: 'current' as target (facebook#1051)
  Remove redundant `function` from export statement (facebook#996)
  Add Gatsby to alternatives (facebook#995)
  Allow webpack 2 as peerDependency in react-dev-utils (facebook#963)
  Remove custom babel-loader cache dir config (facebook#983)
  Check for presence of folders before continuing eject. Closes facebook#939. (facebook#951)
  Fixes facebook#952 (facebook#953)
  Always build before deploying to gh-pages (facebook#959)
  Add collectCoverageFrom option to collect coverage on files without any tests. (facebook#961)
  Catch and noop call to open web browser. (facebook#964)
  Gently nudge users towards https by default (facebook#974)
  Enable compression on webpack-dev-server (facebook#966) (facebook#968)
  Add next.js to Alternatives
  Point people to npm Windows instructions
  Encourage people to try recent npm

# Conflicts:
#	packages/react-scripts/config/webpack.config.dev.js
#	packages/react-scripts/package.json
#	packages/react-scripts/utils/createJestConfig.js
@valscion valscion deleted the re-disable-babel-destructuring branch November 18, 2016 11:57
jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
…ook#1052)

* Update `babel-plugin-transform-object-rest-spread` to v6.19.0

The `babel-plugin-transform-object-rest-spread` v6.19.0 update will
allow us to remove the `babel-plugin-transform-es2015-destructuring` and
`babel-plugin-transform-es2015-parameters` as the object rest spread
transform will now work standalone and not require additional tranforms

* Remove unnecessary babel transform plugins from babel-preset-react-app

The `babel-plugin-transform-object-rest-spread` v6.19.0 update makes
these plugins unnecessary, as v6.19.0 can be used stand-alone
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
…ook#1052)

* Update `babel-plugin-transform-object-rest-spread` to v6.19.0

The `babel-plugin-transform-object-rest-spread` v6.19.0 update will
allow us to remove the `babel-plugin-transform-es2015-destructuring` and
`babel-plugin-transform-es2015-parameters` as the object rest spread
transform will now work standalone and not require additional tranforms

* Remove unnecessary babel transform plugins from babel-preset-react-app

The `babel-plugin-transform-object-rest-spread` v6.19.0 update makes
these plugins unnecessary, as v6.19.0 can be used stand-alone
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
…ook#1052)

* Update `babel-plugin-transform-object-rest-spread` to v6.19.0

The `babel-plugin-transform-object-rest-spread` v6.19.0 update will
allow us to remove the `babel-plugin-transform-es2015-destructuring` and
`babel-plugin-transform-es2015-parameters` as the object rest spread
transform will now work standalone and not require additional tranforms

* Remove unnecessary babel transform plugins from babel-preset-react-app

The `babel-plugin-transform-object-rest-spread` v6.19.0 update makes
these plugins unnecessary, as v6.19.0 can be used stand-alone
@lock lock bot locked and limited conversation to collaborators Jan 22, 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.

5 participants