hughsk / envify

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

Switch from jstransform to acorn #30

Closed zertosh closed 7 years ago

zertosh commented 9 years ago

Hey @hughsk, since jstransform is getting deprecated, I rewrote the plugin using acorn and directly updating the source.

Since this is such a substantial rewrite, if it's cool with you, I'd additionally like to change the API a bit. Basically, move the args parsing to the start of the transform and expose an envify.configure() instead of an envify/custom.js. Moving the args parsing means:

// you can do this now, but it's undocumented
var b = browserify('main.js');
b.transform(envify, {
  NODE_ENV: 'development'
});

// moving the `args` parsing, only the documented way would work
var b = browserify('main.js');
b.transform(envify({
  NODE_ENV: 'development'
}));
hughsk commented 9 years ago

@zertosh the first example's functionality is necessary so that you can still use it from the command-line and package.json files, e.g.:

browserify -t [ envify --NODE_ENV production ]
{
  "browserify": {
    "transform": [
      ["envify", { "NODE_ENV": "production" }]
    ]
  }
}

I think the better solution would be to deprecate envify/custom and stick to the previous approach. envify/custom was written up before browserify transforms had the option of being passed arguments, and isn't really that useful anymore: better to stick with browserify's conventional means of configuration :)

zertosh commented 9 years ago

@hughsk Ah yeah - I didn't think about options from a package.json, my api change suggestion would def break that. Though -t [ ... ] should still work. Anyway, I also added some tests for those two cases. I had to move the tests into a directory since I needed to add fixtures.

Thinking about it some more, I don't think it's possible to deprecate envify/custom without adding some way to specify the environment you want to use. Since browserify always passes an object for argv it's impossible to tell whether someone meant to use that as environment or wanted the default process.env. I'm cool with leaving it as is.

I also ran some benchmarks envifying React:

before:

$ for i in {1..5}; do node bench.js; done
585
592
580
578
585

after:

$ for i in {1..5}; do node bench.js; done
152
143
155
150
154
zertosh commented 9 years ago

@hughsk Since this is such a huge change, I'm not going to merge it w/o your final blessing :innocent:

jmm commented 9 years ago

I was just noticing yesterday how the current API is a bit clunky and found this issue. It would be nice if envify/custom could be done away with. Looking into it now.

jmm commented 9 years ago

@zertosh I see what you mean. I guess this is as good as it gets with the current browserify API. For consistency of API usage I would just suggest maybe not even documenting require('envify') as the way of using it with process.env -- just do require('envify/custom')() (and if a new major version is coming out maybe rename / alias that as something like envify/api and refer to that in the documentation).

ariya commented 7 years ago

PR #52 (which is already merged) superseded this one. I think this PR can be closed now.