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

Removed default options from some Babel plugins #2487

Conversation

adityavohra7
Copy link

This PR removes default config options from some Babel plugins in babel-preset-react-app:

I tested these changes by running npm test in the create-react-app root dir (tests passed), and transpiling a simple file (verified that the output was the same before and after these changes).

Here's the file I used:

// src/foo.js

import 'babel-polyfill';

function *foo() {
    yield 42;
}

BEFORE

$ NODE_ENV=production ./node_modules/.bin/babel src/foo.js yields:

import _regeneratorRuntime from 'babel-runtime/regenerator';

var _marked = [foo].map(_regeneratorRuntime.mark);

import 'babel-polyfill';

function foo() {
    return _regeneratorRuntime.wrap(function foo$(_context) {
        while (1) {
            switch (_context.prev = _context.next) {
                case 0:
                    _context.next = 2;
                    return 42;

                case 2:
                case 'end':
                    return _context.stop();
            }
        }
    }, _marked[0], this);
}

$ NODE_ENV=development ./node_modules/.bin/babel src/foo.js yields:

import _regeneratorRuntime from 'babel-runtime/regenerator';

var _marked = [foo].map(_regeneratorRuntime.mark);

import 'babel-polyfill';

function foo() {
    return _regeneratorRuntime.wrap(function foo$(_context) {
        while (1) {
            switch (_context.prev = _context.next) {
                case 0:
                    _context.next = 2;
                    return 42;

                case 2:
                case 'end':
                    return _context.stop();
            }
        }
    }, _marked[0], this);
}

$ NODE_ENV=test ./node_modules/.bin/babel src/foo.js yields:

'use strict';

var _regenerator = require('babel-runtime/regenerator');

var _regenerator2 = _interopRequireDefault(_regenerator);

require('babel-polyfill');

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var _marked = [foo].map(_regenerator2.default.mark);

function foo() {
    return _regenerator2.default.wrap(function foo$(_context) {
        while (1) {
            switch (_context.prev = _context.next) {
                case 0:
                    _context.next = 2;
                    return 42;

                case 2:
                case 'end':
                    return _context.stop();
            }
        }
    }, _marked[0], this);
}

AFTER

$ NODE_ENV=production ./node_modules/.bin/babel src/foo.js yields:

import _regeneratorRuntime from 'babel-runtime/regenerator';

var _marked = [foo].map(_regeneratorRuntime.mark);

import 'babel-polyfill';

function foo() {
    return _regeneratorRuntime.wrap(function foo$(_context) {
        while (1) {
            switch (_context.prev = _context.next) {
                case 0:
                    _context.next = 2;
                    return 42;

                case 2:
                case 'end':
                    return _context.stop();
            }
        }
    }, _marked[0], this);
}

$ NODE_ENV=development ./node_modules/.bin/babel src/foo.js yields:

import _regeneratorRuntime from 'babel-runtime/regenerator';

var _marked = [foo].map(_regeneratorRuntime.mark);

import 'babel-polyfill';

function foo() {
    return _regeneratorRuntime.wrap(function foo$(_context) {
        while (1) {
            switch (_context.prev = _context.next) {
                case 0:
                    _context.next = 2;
                    return 42;

                case 2:
                case 'end':
                    return _context.stop();
            }
        }
    }, _marked[0], this);
}

$ NODE_ENV=test ./node_modules/.bin/babel src/foo.js yields:

'use strict';

var _regenerator = require('babel-runtime/regenerator');

var _regenerator2 = _interopRequireDefault(_regenerator);

require('babel-polyfill');

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var _marked = [foo].map(_regenerator2.default.mark);

function foo() {
    return _regenerator2.default.wrap(function foo$(_context) {
        while (1) {
            switch (_context.prev = _context.next) {
                case 0:
                    _context.next = 2;
                    return 42;

                case 2:
                case 'end':
                    return _context.stop();
            }
        }
    }, _marked[0], this);
}

Outputs for all envs seem to be the same.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@Timer
Copy link
Contributor

Timer commented Jun 7, 2017

Hi @adityavohra7! We greatly appreciate the contribution.

However, we generally like to opt for explicitness when specifying configurations so that it is easier to understand what we are configuring (and how).
Implicit configuration values can be confusing, especially when looking at the code months down the line.

This also makes it difficult to upgrade package versions, because you're never sure if defaults have changed, etc.

For right now, my stance is to leave this as-is. Do you have an objection?

We eagerly look forward to contributions from you in the future. 😄

@adityavohra7
Copy link
Author

@Timer nope, no objections at all hehe. I was just trying to simplify the file, but I agree that being explicit about the config options makes things easier to track. Feel free to close the PR as un-merged!

I do have two somewhat unrelated followup questions though:

  1. Why does CRA not use babel-plugin-transform-runtime for helpers?
  2. Why does CRA use babel-plugin-transform-regenerator? Wouldn't babel-preset-env take care of transpiling async functions and generators (babel-preset-2015 includes babel-plugin-transform-regenerator)?

@Timer
Copy link
Contributor

Timer commented Jun 8, 2017

Why does CRA not use babel-plugin-transform-runtime for helpers?

We polyfill the usage of required helpers (e.g. Object.assign), meaning there's no reason to have the babel specific (non-global polluting) ones.

Why does CRA use babel-plugin-transform-regenerator? Wouldn't babel-preset-env take care of transpiling async functions and generators (babel-preset-2015 includes babel-plugin-transform-regenerator)?

Fantastic question! I actually forgot why we did this myself, so I did some digging. See #332 for an explaination; effectively the default behavior is async -> generators -> regenerator. Declaring this like so allows us to go from async -> regenerator, bypassing some extra generated boilerplate (and thus shaving off some bytes!).

@Timer Timer closed this Jun 8, 2017
@adityavohra7
Copy link
Author

We polyfill the usage of required helpers (e.g. Object.assign), meaning there's no reason to have the babel specific (non-global polluting) ones.

Hmm isn't that what the "polyfill": false is for though? I think helpers might refer to things like _extend and _classCallCheck (helpers that Babel creates in the transpiled code) . So if this is my file:

// foo.js
export default class Foo {
    constructor() {
        this.foo = 42;
    }
}

Running $ NODE_ENV=production ./node_modules/.bin/babel foo.js with "helpers": false yields:

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var Foo = function Foo() {
    _classCallCheck(this, Foo);

    this.foo = 42;
};

export default Foo;

Running $ NODE_ENV=production ./node_modules/.bin/babel foo.js with "helpers": true yields:

import _classCallCheck from "babel-runtime/helpers/classCallCheck";

var Foo = function Foo() {
    _classCallCheck(this, Foo);

    this.foo = 42;
};

export default Foo;

I can see this being useful for reducing bloat if there are a lot of files with duplicated helpers inlined. I haven't done very many experiments comparing bundle sizes of large apps though so I can't say for sure :(

@Timer
Copy link
Contributor

Timer commented Jun 8, 2017

IIRC babel-runtime/helpers/classCallCheck pulls in other polyfills that are unneeded, like Symbol. If you open a new issue we can talk about (and I would like to!). It's hard to keep track of here. 😄

I believe it is currently the case that every _classCallCheck is duplicated, but we're going to be remedying this when we upgrade to Babel 7 and a newer version of babel-preset-env.

@adityavohra7 adityavohra7 deleted the remove_babel_plugin_defaults branch June 8, 2017 05:38
@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.

3 participants