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

Add env query parameter. #368

Merged
merged 1 commit into from Feb 7, 2017
Merged

Add env query parameter. #368

merged 1 commit into from Feb 7, 2017

Conversation

moimael
Copy link
Contributor

@moimael moimael commented Feb 2, 2017

Allow to override BABEL_ENV/NODE_ENV at loader-level. Useful for isomorphic applications which have separate babel config for client and server.

fix #283

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (not sure if needed here, but i'd like to hear what maintainers think)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Feature

What is the current behavior? (You can also link to an open issue here)
See #283

What is the new behavior?
If forceEnv is set as loader option, use that instead of BABEL_ENV/NODE_ENV

Does this PR introduce a breaking change?

  • No

@codecov-io
Copy link

codecov-io commented Feb 2, 2017

Codecov Report

Merging #368 into master will increase coverage by 0.12%.

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   80.92%   81.04%   +0.12%     
==========================================
  Files           6        6              
  Lines         152      153       +1     
  Branches       33       33              
==========================================
+ Hits          123      124       +1     
  Misses         13       13              
  Partials       16       16
Impacted Files Coverage Δ
src/index.js 82.85% <100%> (+0.24%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9e3702...73128d3. Read the comment docs.

fix babel#283

Signed-off-by: Maël Lavault <mael.lavault@universalavenue.com>
@moimael
Copy link
Contributor Author

moimael commented Feb 6, 2017

Can someone have a look ?

@hiroppy
Copy link
Member

hiroppy commented Feb 7, 2017

@moimael Sorry for the late reply. It looks good to me. cc: @babel/webpack

@danez
Copy link
Member

danez commented Feb 7, 2017

Thanks you

@danez danez merged commit 15881f4 into babel:master Feb 7, 2017
@tleunen
Copy link

tleunen commented Feb 9, 2017

@moimael Could you explain how to use it?
I get the changes you've made in the PR, then try to add the option, but when my app loads, I'm getting this error now:

Module build failed: ReferenceError: [BABEL] /src/index.js: Unknown option: base.forceEnv.

Here's the loader:

{
    test: /\.jsx?$/,
    exclude: /node_modules/,
    use: [
        loader: 'babel-loader',
        options: {
            forceEnv: 'local'
        }
    ]
},

@moimael
Copy link
Contributor Author

moimael commented Feb 9, 2017

How did you install it ? Using file:// ? Did you rebuild it before ?

@tleunen
Copy link

tleunen commented Feb 9, 2017

Manually added the missing piece, but obviously I missed delete userOptions.forceEnv;.

It's good now, thx :)
It was just for a quick testing

@chrisvasz
Copy link
Contributor

This feature isn't working for me. Looking at the code, it looks like this change only affects the cacheIdentifier, but doesn't actually change the env for the Babel transform operation. I had to add something like this to index.js to make babel-loader actually use the specified environment:

var env = userOptions.forceEnv || process.env.BABEL_ENV || process.env.NODE_ENV;
// ...
var tmp = process.env.BABEL_ENV;        // added
process.env.BABEL_ENV = env;            // added
result = transpile(source, options);
process.env.BABEL_ENV = tmp;            // added

I'm happy to send this in a PR, but I wanted to make sure I'm not missing something first.

@moimael
Copy link
Contributor Author

moimael commented Feb 14, 2017

@chrisvasz It is working well for me, so I'm a bit surprised. How do you use it ? I might very well have missed something since it's my first contribution to anything babel related.

EDIT: Ok I see what you mean, I'm quite ashamed to have missed that, I'm indeed only changing the cache identifier. I still don't quite understand why it works for me. I think a PR would be great !

@tleunen
Copy link

tleunen commented Feb 14, 2017

Same for me. it works well.

I don't think it makes sense to update BABEL_ENV. The custom value is passed to babel when compiling, which is why it works.

@chrisvasz
Copy link
Contributor

@tleunen I'm not seeing where the custom value gets passed into Babel. userOptions.forceEnv is used as part of the cacheIdentifier, then deleted before being assigned into options. I dug in because it wasn't working for me... I'm happy to be told that I'm doing something wrong, but looking at the code, I'm not sure how this could work.

@tleunen
Copy link

tleunen commented Feb 14, 2017

Oh, you're right, I missed the stringify function when reading the code.

@moimael
Copy link
Contributor Author

moimael commented Feb 14, 2017

More like forceEnv: 'client'

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.

Setup BABEL_ENV for Webpack2 via loader
7 participants