Create-react-app: In babel-preset-react-app, loose option of @babel/plugin-proposal-object-rest-spread should be enabled

[YimingIsCOLD]

Is this a bug report?

Yes

Did you try recovering your dependencies?

npm version: 6.9.0

Which terms did you search for in User Guide?

words:

babel-preset-react-app
@babel/plugin-proposal-object-rest-spread

Environment

(paste the output of the command here)

Steps to Reproduce

1. Create an app with create-react-app
2. Replace index.js with let { x, y, ...z } = { x: 1, y: 2, a: 3, b: 4 }; let n = { x, y, ...z };
3. npm run build

Expected Behavior

As per doc it should use Object.assign.

(window.webpackJsonp = window.webpackJsonp || []).push([
    [0],
    [
        function(e, n, r) {
            e.exports = r(1);
        },
        function(e, n, r) {
            'use strict';
            r.r(n);
            var t = { x: 1, y: 2, a: 3, b: 4 },
                o = t.x,
                c = t.y,
                i = (function(e, n) {
                    if (null == e) return {};
                    var r,
                        t,
                        o = (function(e, n) {
                            if (null == e) return {};
                            var r,
                                t,
                                o = {},
                                c = Object.keys(e);
                            for (t = 0; t < c.length; t++) (r = c[t]), n.indexOf(r) >= 0 || (o[r] = e[r]);
                            return o;
                        })(e, n);
                    if (Object.getOwnPropertySymbols) {
                        var c = Object.getOwnPropertySymbols(e);
                        for (t = 0; t < c.length; t++)
                            (r = c[t]), n.indexOf(r) >= 0 || (Object.prototype.propertyIsEnumerable.call(e, r) && (o[r] = e[r]));
                    }
                    return o;
                })(t, ['x', 'y']);
            Object.assign({ x: o, y: c }, i);
        },
    ],
    [[0, 1]],
]);
//# sourceMappingURL=main.462a066a.chunk.js.map

However, I need to override the setting to loose: true and useBuiltIns: true to get the expected behavior.

"babel": {
        "presets": [
            "react-app"
        ],
        "plugins": [
            [
                "@babel/plugin-proposal-object-rest-spread",
                {
                    "loose": true,
                    "useBuiltIns": true
                }
            ]
        ]
    },

Actual Behavior

This is the default with no override of @babel/plugin-proposal-object-rest-spread plugin.

(window.webpackJsonp = window.webpackJsonp || []).push([
    [0],
    [
        function(e, t, n) {
            e.exports = n(1);
        },
        function(e, t, n) {
            'use strict';
            function r(e, t, n) {
                return t in e ? Object.defineProperty(e, t, { value: n, enumerable: !0, configurable: !0, writable: !0 }) : (e[t] = n), e;
            }
            n.r(t);
            var o = { x: 1, y: 2, a: 3, b: 4 };
            !(function(e) {
                for (var t = 1; t < arguments.length; t++) {
                    var n = null != arguments[t] ? arguments[t] : {},
                        o = Object.keys(n);
                    'function' === typeof Object.getOwnPropertySymbols &&
                        (o = o.concat(
                            Object.getOwnPropertySymbols(n).filter(function(e) {
                                return Object.getOwnPropertyDescriptor(n, e).enumerable;
                            })
                        )),
                        o.forEach(function(t) {
                            r(e, t, n[t]);
                        });
                }
            })(
                { x: o.x, y: o.y },
                (function(e, t) {
                    if (null == e) return {};
                    var n,
                        r,
                        o = (function(e, t) {
                            if (null == e) return {};
                            var n,
                                r,
                                o = {},
                                c = Object.keys(e);
                            for (r = 0; r < c.length; r++) (n = c[r]), t.indexOf(n) >= 0 || (o[n] = e[n]);
                            return o;
                        })(e, t);
                    if (Object.getOwnPropertySymbols) {
                        var c = Object.getOwnPropertySymbols(e);
                        for (r = 0; r < c.length; r++)
                            (n = c[r]), t.indexOf(n) >= 0 || (Object.prototype.propertyIsEnumerable.call(e, n) && (o[n] = e[n]));
                    }
                    return o;
                })(o, ['x', 'y'])
            );
        },
    ],
    [[0, 1]],
]);
//# sourceMappingURL=main.5bdfaa42.chunk.js.map

Reproducible Demo

(Paste the link to an example project and exact instructions to reproduce the issue.)

[heyimalex]

I'm not sure this is a bug? According to the documentation linked, Object.assign and spread have slightly different behavior. I think we favor spec compliance to performance, even if it's rare to encounter an edge case.

[YimingIsCOLD]

I have tested the options of loose and useBuitIns.

1. loose: false and useBuiltIns: false will use object spread from node_modules/@babel/runtime/helpers/esm/objectSpread.js

2. loose: false and useBuitIns: true will be the same as the first one.

3. loose: true and useBuiltIns: false will use esm_extends from node_modules/@babel/runtime/helpers/esm/extends.js. According to the documentation linked, enabling loose will use the Babel's extends helper and it is correct and if we toggle useBuiltIns too it will use Object.assign. Therefore, just by setting useBuiltIns to true has no effect on anything.

4. loose: true and useBuiltIns: true will use Object.assign.

[ianschmitz]

This portion from the linked documentation is important:

:warning: Please keep in mind that even if they're almost equivalent, there's an important difference between spread and Object.assign: spread defines new properties, while Object.assign() sets them, so using this mode might produce unexpected results in some cases.

As @heyimalex mentioned we prefer sticking to the spec in most cases. Enabling loose mode would deviate from the spec.

[YimingIsCOLD]

Okay. What I should ask is if we do not want loose to be enabled, then why enable useBuiltIns?

[ianschmitz]

That's a good question. I think it may be a historical reason from a couple years back. I don't think the comment above it is true anymore either: https://github.com/facebook/create-react-app/blob/4b5b76b79ffabcafc5fde02f583baceae6bd0446/packages/babel-preset-react-app/create.js#L157-L159.

The above comment isn't true unless loose mode is enabled. It will use the objectSpread helper instead of Object.assign when loose: false.

[heyimalex]

I tracked this down to #902 and it looks like the intention actually was to use Object.assign. So maybe this is a bug? Reading this it seems like the differences between the two are minor, but I don't know enough to say for sure and I don't want to be responsible for a breaking change :')

In either case though we should update by either removing the comment and the option or setting loose: true.

[ianschmitz]

It looks like the current behavior has been the case for quite some (years). Probably best to leave the current behavior which is true to spec, and clean up the config/comments as you mentioned.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[stale[bot]]

This issue has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue.