Skip to content

🏗 Remove the engines section from all package.json files in amphtml#26427

Merged
rsimha merged 1 commit intoampproject:masterfrom
rsimha:2020-10-21-RemoveEngines
Jan 21, 2020
Merged

🏗 Remove the engines section from all package.json files in amphtml#26427
rsimha merged 1 commit intoampproject:masterfrom
rsimha:2020-10-21-RemoveEngines

Conversation

@rsimha
Copy link
Copy Markdown
Contributor

@rsimha rsimha commented Jan 21, 2020

The engines section of package.json has traditionally been used to enforce a known good range of versions of node for local development. This has worked well so far for developers who work on the amphtml repo, where we require an active LTS version of node (and recommend the latest active LTS version of node).

However, we've had multiple instances in the past of the engines section of package.json coming in the way of developer workflows for external contributors who use sub-packages published from within the amphtml repo. See earlier relevant discussions in #19050, #19218, #25161, #25189, #25209, and #26408.

In addition, it has caused failures on Travis CI when node pushes out a new version, causing our package.json files to be outdated until they are manually updated.

When I spoke to @MylesBorins at Open JS last month, he mentioned to me that the engines section is likely to be deprecated soon, and that a better way of enforcing node versions in a repo is to have tests / CI return an error for incompatible versions. We already do this for amphtml via build-system/common/check-package-manager.js.

This PR deletes the engines section from all package.json files in amphtml.

Fixes #26408
Closes #25209
Related to #19050, #19218, #25161, #25189, #25209, and #26408.

@rsimha rsimha self-assigned this Jan 21, 2020
@amp-owners-bot amp-owners-bot bot requested a review from kumagi January 21, 2020 20:49
@rsimha rsimha requested review from Gregable, estherkim, kristoferbaxter and sebastianbenz and removed request for kumagi January 21, 2020 20:51
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jan 21, 2020

@estherkim for infra review.
@Gregable for validator review.
@kristoferbaxter and @sebastianbenz for overall review (in case you can think of a reason not to do this).

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jan 21, 2020

I verbally ran this by @ampproject/wg-infra this afternoon. This PR is good to merge.

@honeybadgerdontcare / @Gregable: I'll leave it to you folks to decide if it's necessary to push a new release for the validator packages gulp-amphtml-validator, amp-validator, amphtml-validator, or amp-validator-webui after this PR is merged.

@rsimha rsimha merged commit fc18cff into ampproject:master Jan 21, 2020
@rsimha rsimha deleted the 2020-10-21-RemoveEngines branch January 21, 2020 21:37
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Sounds prudent to me for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

amphtml-validator Support Node 13 Use renovate to auto-update Node versions in package.json files

6 participants