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

[7.0] Deprecate env option in .babelrc #5276

Closed
hzoo opened this Issue Feb 7, 2017 · 28 comments

Comments

Projects
None yet
@hzoo
Copy link
Member

hzoo commented Feb 7, 2017

## EDIT: We're just going to deprecate now

Fixing merge behavior instead

Original issue details:

http://babeljs.io/docs/usage/babelrc/#env-option

some env issues: #5275, #4539, #4013, #4817

{
  "env": {
    "production": {
      "plugins": ["transform-react-constant-elements"]
    }
  }
}

Options specific to a certain environment are merged into and overwrite non-env specific options.

The env key will be taken from process.env.BABEL_ENV, when this is not available then it uses process.env.NODE_ENV if even that is not available then it defaults to "development".

true that allowing .js may lead to crazy configs but think it's better than confusing with babelrc/env

Because it's JS you could do this in a lot of ways

var env = process.env.BABEL_ENV || process.env.NODE_ENV;
// inline plugin
module.exports = {
  plugins: [
    env === 'development' && "transform-react-constant-elements"
  ].filter(Boolean)
};

https://github.com/facebookincubator/create-react-app/blob/65e63403952f4f3c7e872f707fb3736e339254d9/packages/babel-preset-react-app/index.js#L42-L51

var plugins = [];
if (env === 'development') {
  plugins.push.apply(plugins, ["transform-react-constant-elements"]);
}
module.exports = { plugins };

@hzoo hzoo added the i: discussion label Feb 7, 2017

@jharris4

This comment has been minimized.

Copy link

jharris4 commented Feb 7, 2017

drop the env option entirely and recommend using .babelrc.js using a combiation of ternaries + process.env manually

Is the idea here to support a .babelrc.js file that works the same way as the existing .babelrc file?

@hzoo

This comment has been minimized.

Copy link
Member Author

hzoo commented Feb 7, 2017

Yeah #4892 (added to the description) - it's the same thing

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Feb 7, 2017

Yes, the config would be moved there. You will have then the ability to write JavaScript.

@danez

This comment has been minimized.

Copy link
Member

danez commented Feb 7, 2017

The more I think about this, the more I like it. We could even remove .babelrc support completely and only support the js variant? 😈

Because in every project/lib I know env is used, so a lot of users would need to migrate anyway.

@hzoo hzoo added this to the Babel 7 milestone Feb 7, 2017

@hzoo hzoo changed the title Dropping ENV option in .babelrc? [7.0] Dropping ENV option in .babelrc? Feb 7, 2017

@jharris4

This comment has been minimized.

Copy link

jharris4 commented Feb 7, 2017

I like the idea of .babelrc.js, but this might not be compatible with having a babel config inside package.json.

I'd hate to see that feature be dropped, because it's very convenient to be able to specify & maintain babel plugins and their associated package dependencies all in one file.

The other bonus of using a babel config in package.json is that it doesn't have the side effect that .babelrc files do where the babel config might be unintentionally loaded by some consumer of the project that wants to do its own babel configuration...

@jharris4

This comment has been minimized.

Copy link

jharris4 commented Feb 7, 2017

Also, just to chime in about the only allow a development, production, profiling env options idea, I think this might be a bit too limiting.

Several of our projects make use of custom env values, such as es (to support separate builds in the jsnext:main/module format) and test (to support custom plugins like babel-plugin-rewire that should only ever be included when building/running tests)

@hzoo

This comment has been minimized.

Copy link
Member Author

hzoo commented Feb 7, 2017

The other bonus of using a babel config in package.json is that it doesn't have the side effect that .babelrc files do where the babel config might be unintentionally loaded by some consumer of the project that wants to do its own babel configuration...

It should still because it reads from either babelrc/package.json - you would use babelrc: false to prevent that.

@hzoo

This comment has been minimized.

Copy link
Member Author

hzoo commented Mar 2, 2017

Ok given the widespread use of use of this, let's go with deprecating this feature in 7.0 and then merging babelrc.js.

@hzoo hzoo changed the title [7.0] Dropping ENV option in .babelrc? [7.0] Deprecate env option in .babelrc Mar 2, 2017

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Mar 7, 2017

So now we just want to print a warning about env being deprecated and that it will be removed in v8.0.0 when it's used, yeah? Or does deprecation involve more than that?

@azu

This comment has been minimized.

Copy link
Contributor

azu commented Mar 7, 2017

It will be big breaking change.
I think that it should have graceful migration plan.

remove the merge behavior (each env needs to specify every config value again)
remove development as the default environment

This breanking change may have a potential risk.
Existing .babelrc will continue to work, but it will be broken.
Concernedly. The user can't notice this changes.

IMHO, .babelrc keep going existing behavior and .babelrc.js adopt new algorithm.

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Mar 7, 2017

@azu I think the point of deprecation is that it won't be a breaking change

@hzoo

This comment has been minimized.

Copy link
Member Author

hzoo commented Mar 7, 2017

The description of the issue is for the original issue which was about removing it not deprecating it, I'l update

@azu

This comment has been minimized.

Copy link
Contributor

azu commented Mar 7, 2017

I see.

Is this behavior actually changed in v7 -> v8?
(I alreadly know that some user ignore deprecated warning... My concern will be revisited at that time )

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Mar 8, 2017

@azu No, env will remain as it is now. We will just be recommending using a .babelrc.js file instead of .babelrc with env.

The reason why it's being deprecated and not removed is so that people have time to use the new file type and we don't break existing configurations with env without warning.

@hulkish

This comment has been minimized.

Copy link
Contributor

hulkish commented May 4, 2017

Is babelrc.js always expected to return an object? Ideally... I think it'd nice if it also supported exporting a function. This way you can really gain better control on babel configs for isomorphic apps.

@hulkish

This comment has been minimized.

Copy link
Contributor

hulkish commented May 4, 2017

nevermind.. it appears tis is already the case: https://github.com/babel/babel/blob/7.0/packages/babel-core/src/config/loading/files/configuration.js#L105

  if (typeof options === "function") {
    options = options({
      cache,
      // Expose ".env()" so people can easily get the same env that we expose using the "env" key.
      env: () => cache.using(() => getEnv()),
    });
  } else {
    cache.forever();
  }

woot!

facebook-github-bot added a commit to facebook/react-native that referenced this issue May 12, 2017

Enable JSX source plugin in DEV and integrate it with React DevTools
Summary:
The `env` option has been broken in Babel for a while (babel/babel#4539), and will be deprecated (babel/babel#5276).

I am changing this code to the way Babel < 6.10 interpreted the `env` option. This should fix the `__source` JSX transform which was applied in DEV in the past, but stopped getting applied because of the Babel bug.

I also changed the `filename` passed by Babel to be relative (currently, to `fbsource` but open to other options). This way DEV builds are still reproducible.

This still needs some help from davidaurelio to make the root location more foolproof. I'd also appreciate zertosh taking a look in case there's a better way to "anchor" the relative path. All I need is to for `react-third-party/react-devtools/react-devtools` and `babelTransformer.js` to agree on what the relative path base is.

Closes #13893

Reviewed By: gaearon

Differential Revision: D5035892

Pulled By: davidaurelio

fbshipit-source-id: 19ffeb867d7ed5928e9de05dcec9ba85bf961dd5
@mastilver

This comment has been minimized.

Copy link

mastilver commented May 15, 2017

Shameless plug: I wrote this babel preset a few months ago: https://github.com/mastilver/babel-preset-when

Let's say you are doing that with babel@6:

"env": {
  "development": {
    "presets": [
      [
        "env",
        {
          "targets": {
            "browsers": [
              "last 2 versions"
            ]
          },
          "modules": false
        }
      ]
    ]
  },
  "production": {
    "presets": [
      [
        "env",
        {
          "modules": false
        }
      ]
    ]
  }
}

This will work with babel@7:

{
  "presets": [
    "when": {
      "NODE_ENV=development": {
        "development": {
          "presets": [
            [
              "env",
                {
                  "targets": {
                    "browsers": [
                      "last 2 versions"
                    ]
                  },
                  "modules": false
                }
              ]
            ]
          },
          "production": {
            "presets": [
              [
                "env",
                {
                  "modules": false
                }
              ]
            ]
          }
        }
      }
    }
  ]
}
@danielbayley

This comment has been minimized.

Copy link

danielbayley commented May 19, 2017

@hzoo Just to be clear, is the plan to eventually deprecate .babelrc also, or just env?

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented May 19, 2017

@danielbayley The plan here is to deprecate only env for now.

Maftalion pushed a commit to csatf/react-native that referenced this issue Jun 9, 2017

Enable JSX source plugin in DEV and integrate it with React DevTools
Summary:
The `env` option has been broken in Babel for a while (babel/babel#4539), and will be deprecated (babel/babel#5276).

I am changing this code to the way Babel < 6.10 interpreted the `env` option. This should fix the `__source` JSX transform which was applied in DEV in the past, but stopped getting applied because of the Babel bug.

I also changed the `filename` passed by Babel to be relative (currently, to `fbsource` but open to other options). This way DEV builds are still reproducible.

This still needs some help from davidaurelio to make the root location more foolproof. I'd also appreciate zertosh taking a look in case there's a better way to "anchor" the relative path. All I need is to for `react-third-party/react-devtools/react-devtools` and `babelTransformer.js` to agree on what the relative path base is.

Closes facebook#13893

Reviewed By: gaearon

Differential Revision: D5035892

Pulled By: davidaurelio

fbshipit-source-id: 19ffeb867d7ed5928e9de05dcec9ba85bf961dd5
@kittens

This comment has been minimized.

Copy link
Contributor

kittens commented Jun 12, 2017

This is disappointing. This makes it really hard to cache code from Babel. Including BABEL_ENV in a cache key was easy, but now any potential cache key would have to have some interop to specify the environment variables some plugins may be looking for.

@hzoo

This comment has been minimized.

Copy link
Member Author

hzoo commented Jun 12, 2017

@kittens, @loganfsmyth was working on caching for configs - #5608

const isTest = context.cache(() => (process.env.BABEL_ENV) === 'test');

@OlegLustenko

This comment has been minimized.

Copy link

OlegLustenko commented Aug 7, 2017

Hi, What do you think that updates require a deprecation warning in documentation and at compilation phase, in older versions.

in 7.0 alphas and 6?

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Aug 7, 2017

@OlegLustenko yes, this will require a proper depreciation phase. I don't think it will be part of the first 7 release (or 6.x) but probably in 7.1.

@danielbayley

This comment has been minimized.

Copy link

danielbayley commented Aug 7, 2017

So what happens with package.json.babel?

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Aug 8, 2017

@danielbayley We won't break existing configuration. We are just trying to discourage peoples to use the env key in their configuration (including in the package.json file).

@greggb

This comment has been minimized.

Copy link

greggb commented Oct 13, 2017

As a data point - I had no idea that merging options between envs didn't work. Deprecating in favor of JS would make this much more clear.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Oct 13, 2017

Yeah, the only alternative that comes to mind for me is that we could keep this, but change the logic so that instead of merging, the env blocks replace the root config.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Dec 1, 2017

We've decided that this isn't necessary because we've changed how Babel 7 handles option merging to make this more reasonable for users, and that should be enough.

Config in env is given higher priority than root config items, and instead of just being a weird approach of using both, plugins and presets now merge based on their identity, so you can do

{
  presets: [
    ['env', { modules: false}],
  ],
  env: {
    test: {
      presets: [
         'env'
      ],
    }
  },
}

with BABEL_ENV=test, which would replace the root env config, with the one from test, which has no options.

In Babel 6 things just get combined, so the above would have just loaded the full preset twice.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.