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.

Sign up for GitHub

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

Jump to bottom

[skip ci] Add devEngines to package.json #5312

Merged
merged 2 commits into from Feb 22, 2017
Merged

[skip ci] Add devEngines to package.json #5312

merged 2 commits into from Feb 22, 2017

Conversation

yavorsky
Copy link
Member

Q A
Patch: Bug Fix? n
Major: Breaking Change? n
Minor: New Feature? n
Deprecations? n
Spec Compliancy? n
Tests Added/Pass? n
Fixed Tickets
License MIT
Doc PR n
Dependency Changes n

devEngines is a good way to show environment where build script is expected to work.
Also, going to add for 7.0 brunch without 0.12.

@mention-bot
Copy link

@yavorsky, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @bcoe and @danez to be potential reviewers.

@yavorsky yavorsky changed the title Add devEngines to package.json [skip ci] Add devEngines to package.json Feb 15, 2017
@codecov-io
Copy link

Codecov Report

Merging #5312 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5312   +/-   ##
=======================================
  Coverage   89.44%   89.44%           
=======================================
  Files         204      204           
  Lines        9950     9950           
  Branches     2688     2688           
=======================================
  Hits         8900     8900           
  Misses       1050     1050

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dcc970...2446477. Read the comment docs.

xtuc
xtuc reviewed Feb 15, 2017
View reviewed changes
package.json Outdated
@@ -46,6 +46,10 @@
"through2": "^2.0.0",
"uglify-js": "^2.4.16"
},
"devEngines": {
"node": "0.12 || 4.x || 5.x || 6.x || 7.x",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have drop node 0.12. I guess we can remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current version from master builds on 0.12 🤔 . All works except linting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We support 0.10 on 6.x as well (circle ci) https://github.com/babel/babel/blob/master/circle.yml which we removed in 7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I wouldn't suggest to anyone to develop babel on node <4. I would say for developing people should use latest or LTS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our own development? Oh yeah definetely at least 6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devEngines is not about recommendations but just shows where stuff could technically work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe once preset-env will support engines and we drop es2015 in favor of env. 😏

@hzoo hzoo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 15, 2017
@xtuc
Copy link
Member

xtuc commented Feb 18, 2017

React uses the devEngines key for checking the environment https://github.com/facebook/react/blob/master/package.json#L91

Should we also use this?

@yavorsky
Copy link
Member Author

@xtuc yeah, I thought it as a separate PR. I don't know what's better, script from fbjs or make new for own needs.

@xtuc
Copy link
Member

xtuc commented Feb 19, 2017

@yavorsky I think the FB script is fine (see source).

This will make a new devDependency, but we could use other work they done.

xtuc
xtuc approved these changes Feb 19, 2017
View reviewed changes
Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me on master.

Since 7.0 drops 0.10 and 0.12.

@hzoo hzoo merged commit 02f51fb into babel:master Feb 22, 2017
existentialism pushed a commit that referenced this pull request May 19, 2017
@yavorsky @existentialism
[skip ci] Add devEngines to package.json (#5312)
5c4832c
@sarupbanskota sarupbanskota mentioned this pull request Jul 6, 2017
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danez danez danez left review comments

@hzoo hzoo hzoo left review comments

@xtuc xtuc xtuc approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@yavorsky @mention-bot @codecov-io @xtuc @danez @hzoo