hughsk / envify

:wrench: Selectively replace Node-style environment variables with plain strings.
902 stars 57 forks source link

Envify options in gulp #27

Closed RichardLitt closed 9 years ago

RichardLitt commented 9 years ago

I've run across an issue in transform declaration syntax. The following, using a method, does not work for setting the NODE_ENV:

var b = browserify({
    'cache': {},
    'packageCache': {},
    'fullPaths': true,
    'entries': [paths.jsPath + (argv.path === 'background' ? paths.background : paths.main) + '.js'],
    'noParse': ['react.js', 'jquery.js', 'pdf.combined.js'],
    'transform': [reactify]
  })
  .transform('brfs', { global: true })
  .transform(envify({
    global: true,
    _: 'purge',
    NODE_ENV: 'production'
  }))

However, the following does, calling envify in the transform array:

var b = browserify({
    'cache': {},
    'packageCache': {},
    'fullPaths': true,
    'entries': [paths.jsPath + (argv.path === 'background' ? paths.background : paths.main) + '.js'],
    'noParse': ['react.js', 'jquery.js', 'pdf.combined.js'],
    'transform': [[reactify], ['envify', {'global': true, '_': 'purge', NODE_ENV: 'production'}]]
  })
  .transform('brfs', { global: true })

I don't know the difference, or why one works and not the other. Calling NODE_ENV=production gulp also works, regardless of declaration of envify in the gulp file.

yoshuawuyts commented 9 years ago

Are you requireing envify/custom?

RichardLitt commented 9 years ago

Currently, yes. Sorry, forgot to include that.

yoshuawuyts commented 9 years ago

Ghmm, not sure what is up. Could you try to create a failing test for this?

RichardLitt commented 9 years ago

Can you point me to an example test that you mean? Not familiar with testing frameworks for gulp errors like this.

yoshuawuyts commented 9 years ago

Ghm, I was expecting global: true wouldn't be enabled, but it is. If this is something else than a typo I suspect it doesn't necessarily have something to do with gulp.

slorber commented 9 years ago

@RichardLitt thanks!!! I finally understand my problem so i'll solve yours :) I didn't know anything about this global thing but it seems to do the job.

This does not work:

.transform(envify({
    global: true,
    _: 'purge',
    NODE_ENV: 'production'
  }))

But this will work:

.transform(envify({
    global: true,
    _: 'purge'
   },{global: true});

global is not an Envify option but a Browserify option :)

slorber commented 9 years ago

I've created a PR to improve the doc:

https://github.com/slorber/envify/commit/814b3c44daa13282aa39923ad48a8132a931ed64

RichardLitt commented 9 years ago

Thanks @slorber! I'm a bit confused, though; how does the second one set the NODE_ENV?

slorber commented 9 years ago

@RichardLitt the second one seems to permit to envify the code inside node_modules while the first one does not.

But it seems there are cases where your dependency can handle the envifycation for your. If you are using React, you may be interested by; https://github.com/facebook/react/issues/4107

RichardLitt commented 9 years ago

Interesting. So, I ran a few permutations, and I came up with this table:

  .transform('envify', {_: 'purge', NODE_ENV: 'production'}) // Fails
  .transform('envify', {global: true, _: 'purge', NODE_ENV: 'production'}) // Passes
  .transform('envify', {global: true, _: 'purge', NODE_ENV: 'production'}, {global: true}) // Passes
  .transform('envify', {_: 'purge', NODE_ENV: 'production'}, {global: true}) // Fails
  .transform(envify({_: 'purge', NODE_ENV: 'production'})) // Fails
  .transform(envify({_: 'purge', NODE_ENV: 'production'}), {global: true}) // Passes
  .transform(envify({global: true, _: 'purge', NODE_ENV: 'production'})) // Fails
  .transform(envify({global: true, _: 'purge', NODE_ENV: 'production'}), {global: true}) // Passes

I've got no idea what to make of this. Thanks to @slorber, some of these work, but not all. I'm not sure how much of a bug this is, to be honest.

slorber commented 9 years ago

@RichardLitt it seems to always work when {global:true,...} is the 2nd parameter :) as expected according to the documentation

RichardLitt commented 9 years ago

Excepting: .transform('envify', {global: true, _: 'purge', NODE_ENV: 'production'}), which also passes, although .transform(envify({global: true, _: 'purge', NODE_ENV: 'production'})) does not.

slorber commented 9 years ago

In

.transform('envify', {global: true, _: 'purge', NODE_ENV: 'production'})

global: true is on the second argument of the transform object so it is expected to pass

You pass to browserify transform options that are not browserify transform options but envify custom options: they'll be ignored.

It is the same as calling:

.transform('envify', {global: true})

In

.transform(envify({global: true, _: 'purge', NODE_ENV: 'production'}))

you call transform with a single argument: the envify-custom instance. The option object is passed to envifyCustom and not to the browserify transform function. The global: true is not an envify option so it'll be ignored.

This is the same as calling:

.transform(envify({ _: 'purge', NODE_ENV: 'production'}))

You can make it simpler to understand by decomposing option objects into var:

With envify defaults:

var browserifyTransformOptions = {global: true};
b.transform(  'envify'  ,  browserifyTransformOptions  );

With envify custom:

var envifyCustomOptions = { _: 'purge', NODE_ENV: 'production'};
var browserifyTransformOptions = {global: true};
b.transform(  envify(envifyCustomOptions)  ,  browserifyTransformOptions  );
RichardLitt commented 9 years ago

@slorber Well, gee. I should have caught that, it's fairly obvious when put that way. Thanks for the detailed response, I really appreciated it.

Shall we close this issue now?

slorber commented 9 years ago

guess :) you can do it

jatins commented 8 years ago

How will I specify envifyOptions and browserifyOptions in package.json? I tried this:

"browserify": {
    "transform": [
      "babelify",
      [["envify", {"_": "purge"}], {"global": true}]
    ]
  }

but it throws an error

matthewmueller commented 8 years ago

^ +1 trying to figure this out too

EDIT it's not possible but looks like appending it to the CLI works fine. No clue about the order of transforms though