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

Node.js versions support #253

Merged
merged 12 commits into from Jun 13, 2018
Merged

Conversation

@vostrik
Copy link
Contributor

@vostrik vostrik commented Apr 25, 2018

#75 Added support for Node.js versions

@ai
Copy link
Member

@ai ai commented Apr 25, 2018

CI упал (тесты не полные).

yarn test позволяет прогрнать все проверки локально.

@vostrik
Copy link
Contributor Author

@vostrik vostrik commented Apr 26, 2018

Ага, моя ошибка. Я тесты погонял, не обратил внимание на неполное покрытие, сейчас поправлю.

index.js Outdated
select: function (context, name) {
return nodeReleases
.filter(function (nodeRelease) {
return nodeRelease.lts === true

This comment has been minimized.

@ai

ai Apr 26, 2018
Member

version.lts означает к какому LTS-релизу эта версия привязана. Это никак не связано с поддержкой. Так как LTS-релизы точно так же заканчивают свою поддержку. С друугой стороны в поддержку находятся и не LTS-релизы.

https://github.com/nodejs/Release

This comment has been minimized.

@vostrik

vostrik Apr 26, 2018
Author Contributor

Понял, возьму это расписание:
https://github.com/nodejs/Release/blob/master/schedule.json

This comment has been minimized.

@vostrik

vostrik Apr 26, 2018
Author Contributor

Мы будем из проекта стучаться к этому файлу или должна быть отдельная зависимость, которая будет поставлять расписание?

This comment has been minimized.

@ai

ai Apr 26, 2018
Member

Стучаться точно нельзя.

Надо попросить авторов node-releases добавить эти даннные.

This comment has been minimized.

@vostrik

vostrik Apr 26, 2018
Author Contributor

Создал PR с добавлением расписания в node-releases:
chicoxyzzy/node-releases#2

This comment has been minimized.

@chicoxyzzy

chicoxyzzy May 4, 2018
Contributor

влил

This comment has been minimized.

@vostrik

vostrik May 4, 2018
Author Contributor

@ai Я добавил поддержку запроса "maintained node versions". Но сейчас превышен размер пакета на 1.13 KB (ожидался 7.2 KB).
Можно в node-releases убрать лишние пробелы, это решит проблему.
Делать?

index.js Outdated
}
return 0
})
.pop()

This comment has been minimized.

@ai

ai Apr 26, 2018
Member

Не самый эффективный проход на массиву. Можно сделать без сортировки (которая требует кучу ресурсов) и за один проход. И код будет проще и компактнее.

This comment has been minimized.

@vostrik

vostrik Apr 26, 2018
Author Contributor

Сделал в один проход без сортировки.

This comment has been minimized.

@ai

ai Apr 26, 2018
Member

Огонь 👍

This comment has been minimized.

@vostrik

vostrik Apr 26, 2018
Author Contributor

Йее!

index.js Outdated
})
.pop()
if (targetNodeRelease) {
return name.toLowerCase() + ' ' + targetNodeRelease.version

This comment has been minimized.

@ai

ai Apr 26, 2018
Member

Имя тебя может быть только node

This comment has been minimized.

@vostrik

vostrik Apr 26, 2018
Author Contributor

Поправил

@vostrik
Copy link
Contributor Author

@vostrik vostrik commented Apr 27, 2018

Узнал сегодня насчёт PR в "node-releases" — у chicoxyzzy, возможно, получится посмотреть на выходных или в пн.

@@ -12,7 +12,8 @@
"repository": "browserslist/browserslist",
"dependencies": {
"caniuse-lite": "^1.0.30000830",
"electron-to-chromium": "^1.3.42"
"electron-to-chromium": "^1.3.42",
"node-releases": "^1.0.0-alpha.9"

This comment has been minimized.

@chicoxyzzy

chicoxyzzy May 4, 2018
Contributor

Тут теперь alpha.10. Когда стабилизируем формат данных в node-releases, выпустим 1.0.

This comment has been minimized.

@vostrik

vostrik May 4, 2018
Author Contributor

Спасибо! @chicoxyzzy скажи, пожалуйста, когда будет публикация версии alpha.10?

This comment has been minimized.

@chicoxyzzy

chicoxyzzy May 4, 2018
Contributor

запаблишил. также добавил @ai права на пакет во избежание бас фактора

This comment has been minimized.

@vostrik

vostrik May 4, 2018
Author Contributor

Спасибо большое!

@ai
Copy link
Member

@ai ai commented May 4, 2018

Не поможет. Размер считается после uglify и gzip.

Пока просто увеличь лимит. Потом подумаем, как эффективно запаковать данные.

@chicoxyzzy

@vostrik
Copy link
Contributor Author

@vostrik vostrik commented May 4, 2018

Готово. Увеличил допустимый размер до 9.6 KB.
В целом, все правки выполнил.

@ai
Copy link
Member

@ai ai commented May 4, 2018

Отлично. Приеду домой и отмечу как выполненное.

Но до того как принять, надо поговорить с Babel.

@ai
Copy link
Member

@ai ai commented May 5, 2018

I asked @yavorsky and @existentialism for code review in Babel Slack. Sad that we used Russian (but most of Russian comments above is just about fixing project’s linter warnings)

@ai
ai approved these changes May 9, 2018
@ai
Copy link
Member

@ai ai commented May 9, 2018

@vostrik I ill merge it when will finish my next talk preparation (sorry, have no time).

But I marked Cult of Martians task solved by you. Great work 👍

@vostrik
Copy link
Contributor Author

@vostrik vostrik commented May 9, 2018

Thank you!

@ai
Copy link
Member

@ai ai commented Jun 1, 2018

I remember about this PR. Just have a limited time because of PiterCSS

@ai ai changed the base branch from master to v4 Jun 13, 2018
@ai ai merged commit d30c299 into browserslist:v4 Jun 13, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ai ai mentioned this pull request Jun 13, 2018
ai added a commit that referenced this pull request Jun 21, 2018
* Add "node-releases" to dependencies

* Add Node.js versions support

* Use simple string 'node' in selected version

* Handle case without raised error with flag ignoreUnknownVersions = true

* Change test name

* Find relevant node release in a more effective way

* Fix codestyle

* Update to node-releases@1.0.0-alpha.10

* Support "maintained node version" query

* Mock Node.js query tests for 100% coverage

* Increase allowed package size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants