Skip to content

Upgrade node from ^10.0.0 to ^12.0.0#25161

Merged
estherkim merged 4 commits intoampproject:masterfrom
estherkim:bump-node
Oct 21, 2019
Merged

Upgrade node from ^10.0.0 to ^12.0.0#25161
estherkim merged 4 commits intoampproject:masterfrom
estherkim:bump-node

Conversation

@estherkim
Copy link
Copy Markdown
Collaborator

To match node v12 being the active LTS version as of 10/21/19: https://nodejs.org/en/about/releases/

@estherkim estherkim requested a review from rsimha October 21, 2019 18:50
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

"main": "index.js",
"engines": {
"node": "^10.0.0",
"node": "^12.0.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To increase resiliency, it might be worth allowing the current and the next LTS version (^12 || ^14) in a future PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some developers reported on slack they are no longer able to use amphtml-validator with nodejs 10.0. And they cannot upgrade to 12.0 soon.

Can we update this to support >=10.0 ?

Sorry, I do not have much background about this upgrade.

Copy link
Copy Markdown
Member

@twifkak twifkak Oct 22, 2019

Choose a reason for hiding this comment

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

+1 if feasible. FWIW, v10 is still considered "active LTS" until 2020-04-01 and not EOL until 2021-04-30 per https://nodejs.org/en/about/releases/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It sounds reasonable to support all currently Active LTS versions (as of today, 10 and 12). So maybe this should be ^10 || ^12 || ^14?

(This is different from >= 10 because we want to skip major versions that will never be an LTS version.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

PR here for >=10.0.0, could update to the suggestion above. #25208

Copy link
Copy Markdown
Contributor

@rsimha rsimha Oct 22, 2019

Choose a reason for hiding this comment

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

To build future resiliency, we can have renovate auto-update the engines section of all our package.json files: https://docs.renovatebot.com/node/

I'll file a tracking issue. Edit: Filed #25209 to track this.

"@jridgewell/resorcery": "0.0.3",
"acorn-globals": "4.3.4",
"amphtml-validator": "1.0.26",
"amphtml-validator": "1.0.23",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@honeybadgerdontcare Any concerns with rolling back temporarily?

@estherkim estherkim merged commit c01bac5 into ampproject:master Oct 21, 2019
@estherkim estherkim deleted the bump-node branch October 21, 2019 19:27
@cathyxz
Copy link
Copy Markdown
Contributor

cathyxz commented Oct 21, 2019

Thank you Esther!!!!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants