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

support env in babelrc #8963

Merged
merged 3 commits into from Aug 7, 2017
Merged

support env in babelrc #8963

merged 3 commits into from Aug 7, 2017

Conversation

hexsprite
Copy link
Contributor

This PR implements support for the env key in .babelrc files.

It looks first for BABEL_ENV then NODE_ENV then finally defaults to development per the Babel documented behavior.

Rationale: be able to add additional babel plugins/presets for testing environment for example (eg. babel-plugin-istanbul)

I wasn't sure where to add tests as I did not see any existing tests for the functionality. Please let me know your feedback.

@apollo-cla
Copy link

@hexsprite: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -300,6 +300,16 @@ BCp._inferHelper = function (
merge(babelOptions, babelrc, "presets");
merge(babelOptions, babelrc, "plugins");

const babelEnv = (process.env.BABEL_ENV ||
process.env_NODE_ENV ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be process.env.NODE_ENV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so. From the babel docs:

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".

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@hexsprite Ben's referring to the typo in your process.env_NODE_ENV reference (note the _).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, woops. Got it.

@benjamn
Copy link
Contributor

benjamn commented Aug 2, 2017

I wasn't sure where to add tests as I did not see any existing tests for the functionality. Please let me know your feedback.

I would recommend adding env to some of the .babelrc files in the modules test app, or creating new local packages with new .babelrc files in that app.

@benjamn benjamn changed the base branch from devel to release-1.5.2 August 7, 2017 17:23
@benjamn benjamn merged commit 77ecbab into meteor:release-1.5.2 Aug 7, 2017
benjamn added a commit that referenced this pull request Aug 7, 2017
The modules test app appears to be running with process.env.NODE_ENV equal
to "production" on Circle CI: https://circleci.com/gh/meteor/meteor/5030.

Enabling this transform in production as well as development is fine
because we primarily want to test that plugins from the "env" section of
.babelrc are respected, regardless of the value of process.env.NODE_ENV.
Using different plugins in production might be worth testing, too, but
that's less critical.

Follow-up to #8963.
benjamn added a commit that referenced this pull request Sep 26, 2017
When the ecmascript package version was last bumped in
18e4c17, it appears that
babel-compiler@6.20.0 had not yet been published, so ecmascript was
published with a copy of that compiler plugin that did not support the
"env" property of .babelrc files (#8963).

Bumping again and republishing in hopes of fixing that problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants