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 different environment #88

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@maksimsemenov
Copy link
Contributor

commented Dec 16, 2016

My solution for #76

@ai ai referenced this pull request Dec 16, 2016

Closed

Added different environments #89

@ai

This comment has been minimized.

Copy link
Member

commented Dec 16, 2016

Looks good. I will accept it in the evening.

@@ -3,6 +3,9 @@ var path = require('path');
var fs = require('fs');

var FLOAT = /^\d+(\.\d+)?$/;
var env = 'development';
// console.log(process.env.BROWSERSLIST_ENV, process.env.NODE_ENV, env);
// console.log('test', process.env.BROWSERSLIST);

This comment has been minimized.

Copy link
@ai

ai Dec 19, 2016

Member

Next time always check PR for unnecessary comments before send it :)

if ( envConf !== false ) {
selections = envConf;
} else {
selections = browserslist.defaults;

This comment has been minimized.

Copy link
@ai

ai Dec 19, 2016

Member

You get browserslist.defaults twice: here and in configForEnv. Seems like one of them is unnecessary.

return i !== '';
});
.split('[')
.reduce(function (c, ch) {

This comment has been minimized.

Copy link
@ai

ai Dec 19, 2016

Member

reduce is tricky code, it is better to have a state machine with will read every line and put it to object. And if line contains [ it will change a current env.

@ai

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

I will try to make Frankenstein from your PR and #89. Because everyone have own benefits.

@@ -135,7 +133,7 @@ assert-plus@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/assert-plus/-/assert-plus-1.0.0.tgz#f12e0f3c5d77b0b1cdd9146942e4e96c1e4dd525"

async@1.x, async@^1.4.0, async@^1.4.2:
async@^1.4.0, async@^1.4.2, async@1.x:

This comment has been minimized.

Copy link
@ai

ai Dec 19, 2016

Member

Don’t update yarn.lock if your fix doesn’t need it. “One PR — one issue” is good idea :D.

expect(browserslist(null, { path: withEnvB, env: 'test' }))
.toEqual(['ie 11', 'ie 10']);
});
it('return correct config for direct env from package', () => {

This comment has been minimized.

Copy link
@ai

ai Dec 19, 2016

Member

Always try to copy origin code style. I used a \n\n between tests (it is a common style)

@ai

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

Merged manually. I will use your PR as “solution” on Cult of Martians, because you was first and you have clear name and photo in GitHub profile :).

@ai ai referenced this pull request Dec 19, 2016

Closed

Add different environment #76

@ai ai closed this Dec 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.