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

Enable useBuiltIns option on object-rest-spread #902

Merged
merged 4 commits into from
Nov 21, 2016

Conversation

existentialism
Copy link
Contributor

Addresses #899

@@ -14,7 +14,10 @@ const plugins = [
// class { handleClick = () => { } }
require.resolve('babel-plugin-transform-class-properties'),
// { ...todo, completed: true }
require.resolve('babel-plugin-transform-object-rest-spread'),
[require.resolve('babel-plugin-transform-object-rest-spread'), {
// Use Object.assign directly, instead of extends helper
Copy link
Contributor

@gaearon gaearon Oct 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a note that this requires Object.assign polyfill, and do it in README of the preset too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hzoo
Copy link

hzoo commented Nov 16, 2016

Based on #399 it looks like it's already being polyfilled so this wouldn't be a breaking change?

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

It's polyfilled for Create React App users, but not for people who use standalone plugin.
It should be a major bump for the Babel plugin (cc @fson).

@fson fson added this to the 0.8.0 milestone Nov 20, 2016
@existentialism
Copy link
Contributor Author

Sidenote, in a separate PR we can also enable useBuiltIns for transform-react-jsx

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

@existentialism Want to do it here as well?

@existentialism
Copy link
Contributor Author

@gaearon sure!

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

I tried using spread locally on your branch and I still see the _extends helper. Do you need to also update Babel package versions or something?

@existentialism
Copy link
Contributor Author

Strange, I'm seeing the following snippet from App.js:

render() {
  const a = {
    foo: 'bar'
  };

  const b = {
    ...a,
    baz: 'bat'
  };

  return (
    <div className="App" {...b}>

Without PR:

// Helper above:
var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

_createClass(App, [{
  key: 'render',
  value: function render() {
    var a = {
      foo: 'bar'
    };

    var b = _extends({}, a, {
      baz: 'bat'
    });

    return _react2.default.createElement(
      'div',
      _extends({ className: 'App' }, b, {

With PR:

// No helper above...

_createClass(App, [{
  key: 'render',
  value: function render() {
    var a = {
      foo: 'bar'
    };

    var b = Object.assign({}, a, {
      baz: 'bat'
    });

    return _react2.default.createElement(
      'div',
      Object.assign({ className: 'App' }, b, {

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

@fson Yarn is failing the build here, any ideas why?

@fson
Copy link
Contributor

fson commented Nov 20, 2016

Weird, maybe a temporary glitch?

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

Yea. Leaving this to you to review.

@fson
Copy link
Contributor

fson commented Nov 21, 2016

Looks good to me!

@fson fson merged commit 9c45b25 into facebook:master Nov 21, 2016
@existentialism existentialism deleted the usebuiltins branch November 21, 2016 22:09
jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
* Enable useBuiltIns option on object-rest-spread

* note polyfill requirement

* Enable useBuiltIns with transform-react-jsx

* Add direct ref to transform-react-jsx
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
* Enable useBuiltIns option on object-rest-spread

* note polyfill requirement

* Enable useBuiltIns with transform-react-jsx

* Add direct ref to transform-react-jsx
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
* Enable useBuiltIns option on object-rest-spread

* note polyfill requirement

* Enable useBuiltIns with transform-react-jsx

* Add direct ref to transform-react-jsx
@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