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 load statistic from browserslist-stats.json #86

Merged
merged 7 commits into from Dec 15, 2016

Conversation

Projects
None yet
2 participants
@AleshaOleg
Copy link
Contributor

commented Dec 14, 2016

No description provided.

README.md Outdated
@@ -157,6 +157,7 @@ If you have a website, you can query against the usage statistics of your site:
```js
browserslist('> 5% in my stats', { stats: 'path/to/the/stats.json' });
```
5. Import statistic from `browserslist-stats.json` in project folder

This comment has been minimized.

Copy link
@ai

ai Dec 14, 2016

Member

It is not a separated step. It is just part of 4 step. Also feel free to rewrite section to make it more clear.

index.js Outdated
return;
}

var files = fs.readdirSync(startPath), filename, stat, currFile;

This comment has been minimized.

Copy link
@ai

ai Dec 14, 2016

Member

Mixed variable difinition with values var a = 1 and without var a is prohibited in this project :).

index.js Outdated
if ( opts.stats || process.env.BROWSERSLIST_STATS ) {
statsExist = true;
} else {
(function searchStatsFile(startPath) {

This comment has been minimized.

Copy link
@ai

ai Dec 14, 2016

Member

Too tricky code. It is better to use while from config loader without a recursion. Recursion is always hard to understand.

This comment has been minimized.

Copy link
@AleshaOleg

AleshaOleg Dec 15, 2016

Author Contributor

@ai but as I understood from task, it should to search file in any folder of project folder. Or same while loop as in config search will be enough?

This comment has been minimized.

Copy link
@ai

ai Dec 15, 2016

Member

@AleshaOleg if we process /a/b/c/file.css we should check stat file in /a/b/c, /a/b, /a and /.

And we already do this “recursion” search in config loading (but without recursion in code).

This comment has been minimized.

Copy link
@AleshaOleg

AleshaOleg Dec 15, 2016

Author Contributor

@ai ok, thinking we need to found this file in all children folders:) Ok, got it

index.js Outdated
}

var files = fs.readdirSync(startPath), filename, stat, currFile;
for (var i = 0; i < files.length; i++ ) {

This comment has been minimized.

Copy link
@ai

ai Dec 14, 2016

Member

This project code style need to have space after ( in if/for/while.

"7": 0.4,
"8": 1.5
}
}

This comment has been minimized.

Copy link
@ai

ai Dec 14, 2016

Member

No new line at the end of file.

@@ -40,3 +41,9 @@ it('works alongside global usage query', () => {
var list = browserslist('> 10% in my stats, > 1%', { stats: usage });
expect(list.length > 1).toBeTruthy();
});

it('take stats from browserslist-stats.json', () => {
var data = JSON.parse(fs.readFileSync(statsFile));

This comment has been minimized.

Copy link
@ai

ai Dec 14, 2016

Member

You pass a stats by stats argument in this test case. It doesn’t test that you look up in directories.

Look how I test a config file.

README.md Outdated
or `BROWSERSLIST_STATS` environment variable:
4. Give it to Browserslist by `stats` option,
`BROWSERSLIST_STATS` environment variable
or import from `browserslist-stats.json` in project folder:

This comment has been minimized.

Copy link
@ai

ai Dec 15, 2016

Member

I think “save to browserslist-stats.json” will be much more clear, than “import” (also feel free to put this way as first way).

This comment has been minimized.

Copy link
@AleshaOleg

AleshaOleg Dec 15, 2016

Author Contributor

@ai just updated, please review README again.

@@ -40,3 +40,7 @@ it('works alongside global usage query', () => {
var list = browserslist('> 10% in my stats, > 1%', { stats: usage });
expect(list.length > 1).toBeTruthy();
});

it('take stats from browserslist-stats.json', () => {
expect(browserslist('> 5% in my stats')).toEqual(['ie 8']);

This comment has been minimized.

Copy link
@ai

ai Dec 15, 2016

Member

Why this test are working? You didn’t pass a file option, so Browserslist should not know where to find.

index.js Outdated
@@ -519,4 +541,6 @@ browserslist.queries = {
}
}());

browserslist();

This comment has been minimized.

Copy link
@ai

ai Dec 15, 2016

Member

Why you call empty function here?

index.js Outdated
if ( opts.stats || process.env.BROWSERSLIST_STATS ) {
var statsFile;

var getStats = function () {

This comment has been minimized.

Copy link
@ai

ai Dec 15, 2016

Member

Code become better but still too tricky. What do you think about this:

var stats = opts.stats || process.env.BROWSERSLIST_STATS
if ( !stats ) {
  // search in files
}
if ( !stats ) {
  // show error
} else {
  // use custom stats
}

In this case you don’t need getStats function and could read code from top to bottom, without jumping in file.

This comment has been minimized.

Copy link
@AleshaOleg

AleshaOleg Dec 15, 2016

Author Contributor

@ai ESLint thinks it's a complexity in this case, trying come up with a solution

This comment has been minimized.

Copy link
@ai

ai Dec 15, 2016

Member

@AleshaOleg you can always change complexity settings, you you understand that they are wrong in this case.

Or move it to var stats = browserslist.loadStats()

This comment has been minimized.

Copy link
@AleshaOleg

AleshaOleg Dec 15, 2016

Author Contributor

@ai so, i pushed a new changes - but some code is still in other function - because of complexity browserslist function. If I push code of getStat to this function, complexity will be 22, and that's why i now need to have this function. So, I'm waiting to your opinion, how browserslist function should be to look like

@ai ai merged commit 4ff828e into browserslist:master Dec 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.