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

Switch from jstransform to acorn #30

Closed
wants to merge 3 commits into from
Closed

Switch from jstransform to acorn #30

wants to merge 3 commits into from

Conversation

zertosh
Copy link
Collaborator

@zertosh zertosh commented Jun 13, 2015

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
Copy link
Owner

hughsk commented Jun 13, 2015

@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
Copy link
Collaborator Author

zertosh commented Jun 13, 2015

@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
Copy link
Collaborator Author

zertosh commented Jun 19, 2015

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

@jmm
Copy link

jmm commented Jun 19, 2015

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
Copy link

jmm commented Jun 19, 2015

@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).

@btd btd mentioned this pull request Jul 8, 2015
@ariya ariya mentioned this pull request Oct 27, 2016
@ariya
Copy link
Contributor

ariya commented Dec 6, 2016

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

@zertosh zertosh closed this Dec 6, 2016
@zertosh zertosh deleted the switch-to-acorn branch December 6, 2016 05:05
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.

4 participants