Drop nodejs 6 for next (Fastify 3) #1713

smartiniOnGitHub · 2019-06-19T12:00:22Z

Hi all,
this is mainly an update/cleanup on documentation (and CI tools) for removing Node.js support in Fastify v3 (in its dev branch 'next'), related to the issue#1647 . With this, minimum Node.js version set is 8 LTS (8.9.0).

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

smartiniOnGitHub · 2019-06-19T12:02:30Z

Hi all, note that I put here some TODO because not sure you prefer that I update code here (I think it's good to do here), just for a check ... tell me so during next days I can finish these changes and then continue with the other issue related to other code changes. Thanks. Bye

test/async-await.test.js

test/fluent-schema.test.js

test/hooks.test.js

test/http2/http2.test.js

test/reply-error.test.js

README.md

docs/Benchmarking.md

lib/reply.js

package.json

mcollina · 2019-06-21T14:16:08Z

Would you mind changing the yarn config for azure pipelines to require node >= 8.16.0?

node_8_x:
        node_version: >=8.16.0

smartiniOnGitHub · 2019-06-21T16:22:19Z

@mcollina sure, I'll do that change, thanks for the suggestion.
Sorry another question, wouldn-t it be better to run Azure pipelines on Ubuntu-18.04 (LTS) instead of previous LTS (16.04) ? If yes I'll do even this change. Bye

mcollina · 2019-06-21T16:38:16Z

Sorry another question, wouldn-t it be better to run Azure pipelines on Ubuntu-18.04 (LTS) instead of previous LTS (16.04) ? If yes I'll do even this change. Bye

Yes it would be! Would you mind sending a PR against master for that?

smartiniOnGitHub · 2019-06-21T16:55:24Z

@mcollina sure, I'll raise another PR for that change (for master).

smartiniOnGitHub · 2019-06-21T22:49:32Z

@mcollina all done, thanks again for all the help on this.
As always I made a lot of commits, hope PR merge with squash is good the same for you. Bye

mcollina

LGTM, good work!

lib/reply.js

smartiniOnGitHub · 2019-06-22T16:39:46Z

all done, thanks to all for the help

docs/LTS.md

docs/HTTP2.md

build/azure-pipelines-yarn-template.yml

delvedor · 2019-06-24T19:11:34Z

Would you mind changing the yarn config for azure pipelines to require node >= 8.16.0?

@mcollina why?

smartiniOnGitHub · 2019-06-24T20:54:01Z

in last commit/push all seems good in Azure, but 3 builds failed due to this error (strange):
"VS30063: You are not authorized to access https://tcmprodweu1.vstmr.visualstudio.com."
Could someone re-execute them, just to be sure that all is good the same ? Thanks a lot.

mcollina · 2019-06-26T09:25:38Z

Would you mind changing the yarn config for azure pipelines to require node >= 8.16.0?

@mcollina why?

Our Azure Pipeline contact said so! ;).

mcollina · 2019-06-26T09:26:10Z

@delvedor can you LGTM this?

@smartiniOnGitHub would you mind squashing your commits to retrigger CI, I think the failure should go away (and it's not related anyway).

add TODO for code updates/cleanup (check if do here or if in the other issue/PR) Update README.md Co-Authored-By: Matteo Collina <matteo.collina@gmail.com> Update docs/Benchmarking.md Co-Authored-By: Matteo Collina <matteo.collina@gmail.com> Update package.json Co-Authored-By: Matteo Collina <matteo.collina@gmail.com> revert version update getHeader to use hasHeader (available since Node.js 7.7.x) and remove fallback implementation try yarn builds for Node.js 8 aligned with requirements in 'package.json', to check if it works with version range align even npm azure pipelines to those of yarn (Node.js 8.x requirements) update code to Node.js 8 as minimum version (remove TODO etc) update code to Node.js 8 as minimum version (remove TODO etc) update code to Node.js 8 as minimum version (remove TODO etc) move getHeader implementation directly in Reply.prototype.getHeader drop Node.js 11 from doc and from CI on Azure (yarn and npm) (try to) simplify info on HTTP2 support CI on Azure, specify version range in a way more aligned to npm syntax

smartiniOnGitHub · 2019-06-26T12:20:22Z

@mcollina yes, retried now (after the squash) and all builds are successful now, thanks for the suggestion.

Eomm

A question in my mind since this PR is the first (ready to merge) big split from v2:
which is the right process to keep the next branch aligned with the features added in v2/master?
Opening a PR from master to next or rebasing periodically?

Because the next branch starts in between v2.3/2.4

smartiniOnGitHub · 2019-06-27T07:49:57Z

I think a rebase of next on top of master would be good, but Lead Fastify developers can say more ...

mcollina · 2019-06-28T08:31:51Z

I think a rebase of next on top of master would be good, but Lead Fastify developers can say more ...

That'd be ok. it's not urgent anyway.

delvedor

I think we should keep the async hook test in its own file.
Currently, the hooks test file is about 3000+ lines long.

mcollina

Some nits.

Also, would you mind restoring test/hooks-async.js and rename it to test/hooks-async.test.js and make that runnable?

test/http2/plain.test.js

test/http2/secure-with-fallback.test.js

test/http2/secure.test.js

test/http2/unknown-http-method.test.js

…ike others; remove its tests from 'hooks.test.js'

smartiniOnGitHub · 2019-06-28T16:06:59Z

hooks-async.js

done, yes now it looks better (big test file shorter, and this with specific tests restored and updated)

…really needed

smartiniOnGitHub · 2019-06-28T16:42:46Z

@mcollina latest changes done (as per your request), hope all is good now. Bye

mcollina

LGTM

mcollina · 2019-06-29T06:08:23Z

Great work Sandro!

delvedor

LGTM

* update requirements to Node.js 8 LTS or later; update docs add TODO for code updates/cleanup (check if do here or if in the other issue/PR) Update README.md Co-Authored-By: Matteo Collina <matteo.collina@gmail.com> Update docs/Benchmarking.md Co-Authored-By: Matteo Collina <matteo.collina@gmail.com> Update package.json Co-Authored-By: Matteo Collina <matteo.collina@gmail.com> revert version update getHeader to use hasHeader (available since Node.js 7.7.x) and remove fallback implementation try yarn builds for Node.js 8 aligned with requirements in 'package.json', to check if it works with version range align even npm azure pipelines to those of yarn (Node.js 8.x requirements) update code to Node.js 8 as minimum version (remove TODO etc) update code to Node.js 8 as minimum version (remove TODO etc) update code to Node.js 8 as minimum version (remove TODO etc) move getHeader implementation directly in Reply.prototype.getHeader drop Node.js 11 from doc and from CI on Azure (yarn and npm) (try to) simplify info on HTTP2 support CI on Azure, specify version range in a way more aligned to npm syntax * restore 'hooks-async.js' and set the '.test.js' extension to run it like others; remove its tests from 'hooks.test.js' * remove direct reference to main fastify source from https tests, not really needed

github-actions · 2022-02-10T09:15:37Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mcollina reviewed Jun 19, 2019

View reviewed changes

mcollina approved these changes Jun 22, 2019

View reviewed changes

delvedor reviewed Jun 22, 2019

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

L2jLiga reviewed Jun 24, 2019

View reviewed changes

docs/LTS.md Outdated Show resolved Hide resolved

L2jLiga reviewed Jun 24, 2019

View reviewed changes

docs/HTTP2.md Outdated Show resolved Hide resolved

L2jLiga reviewed Jun 24, 2019

View reviewed changes

build/azure-pipelines-yarn-template.yml Outdated Show resolved Hide resolved

Eomm added internals Change that won't impact the surface API. v3.x Issue or pr related to Fastify v3 labels Jun 26, 2019

Eomm approved these changes Jun 26, 2019

View reviewed changes

delvedor reviewed Jun 28, 2019

View reviewed changes

mcollina requested changes Jun 28, 2019

View reviewed changes

test/http2/plain.test.js Outdated Show resolved Hide resolved

test/http2/secure-with-fallback.test.js Outdated Show resolved Hide resolved

test/http2/secure.test.js Outdated Show resolved Hide resolved

test/http2/unknown-http-method.test.js Outdated Show resolved Hide resolved

restore 'hooks-async.js' and set the '.test.js' extension to run it l…

006e81e

…ike others; remove its tests from 'hooks.test.js'

remove direct reference to main fastify source from https tests, not …

b7b9f0b

…really needed

mcollina approved these changes Jun 29, 2019

View reviewed changes

delvedor approved these changes Jul 3, 2019

View reviewed changes

mcollina merged commit d6b621d into fastify:next Jul 3, 2019

mcollina mentioned this pull request Nov 24, 2019

Rebase next #1953

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022

Uh oh!

Drop nodejs 6 for next (Fastify 3) #1713

Drop nodejs 6 for next (Fastify 3) #1713

Uh oh!

Conversation

smartiniOnGitHub commented Jun 19, 2019

Checklist

Uh oh!

smartiniOnGitHub commented Jun 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina commented Jun 21, 2019

Uh oh!

smartiniOnGitHub commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jun 21, 2019

Uh oh!

smartiniOnGitHub commented Jun 21, 2019

Uh oh!

smartiniOnGitHub commented Jun 21, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smartiniOnGitHub commented Jun 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delvedor commented Jun 24, 2019

Uh oh!

smartiniOnGitHub commented Jun 24, 2019

Uh oh!

mcollina commented Jun 26, 2019

Uh oh!

mcollina commented Jun 26, 2019

Uh oh!

smartiniOnGitHub commented Jun 26, 2019

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

smartiniOnGitHub commented Jun 27, 2019

Uh oh!

mcollina commented Jun 28, 2019

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smartiniOnGitHub commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smartiniOnGitHub commented Jun 28, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 29, 2019

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

smartiniOnGitHub commented Jun 21, 2019 •

edited

Loading

smartiniOnGitHub commented Jun 28, 2019 •

edited

Loading