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

build: [email protected] and [email protected] #4338

Merged

Conversation

3imed-jaberi
Copy link
Member

  • Support Node.js 13.x.
  • Support Node.js 14.x.

@dougwilson
Copy link
Contributor

Thank you @3imed-jaberi ! I would suggest splitting these up into two different PRs. Part of adding a new major version of supported Node.js to our CI, we need to go through every dependency in our dependency tree and see if they are testing against that version of Node.js, or test locally against that version of Node.js. This ensures that we are actually supporting it. Many of our complicated dependencies we do not copy their test suites into Express, so we don't know if the features they are providing to Express fully function without running the test suites of those dependencies in those version of Node.js ourselves.

If you're going to make a PR, I would suggest listing out all the dependencies, then note down the version of that dependency you tested into the new major version of Node.js you're adding and if any issues were encountered or not.

@dougwilson dougwilson added the pr label Jul 3, 2020
@3imed-jaberi
Copy link
Member Author

Thank you @3imed-jaberi ! I would suggest splitting these up into two different PRs. Part of adding a new major version of supported Node.js to our CI, we need to go through every dependency in our dependency tree and see if they are testing against that version of Node.js, or test locally against that version of Node.js. This ensures that we are actually supporting it. Many of our complicated dependencies we do not copy their test suites into Express, so we don't know if the features they are providing to Express fully function without running the test suites of those dependencies in those version of Node.js ourselves.

If you're going to make a PR, I would suggest listing out all the dependencies, then note down the version of that dependency you tested into the new major version of Node.js you're adding and if any issues were encountered or not.

@dougwilson, thank you bro again for your quick reaction with my PRs, I will do whatever you requested .. Then I will love to hear from you about opening another PR or stay here.

@dougwilson
Copy link
Contributor

It's no problem! I am happy to see folks come and help with these types of tasks :) !

@3imed-jaberi
Copy link
Member Author

@dougwilson, I have checked all the dependencies, most of them are the same as the previous version, and as for the dependencies that we have updated, they work fine locally.

Support through the CI:

@dougwilson
Copy link
Contributor

I have checked all the dependencies, most of them are the same as the previous version

Thank you! To be clear, I'm not sure what you're saying if you can help me out. That seems like a very small subset of our dependencies. Does, for example, the send dependency work on Node.js 13 and 14? I didn't see you list that one, and it's one of our critical dependencies to check out.

For your check list, what did you use to determine which dependencies to list there vs not list?

@3imed-jaberi
Copy link
Member Author

3imed-jaberi commented Jul 4, 2020

Bro, few minutes and I will list all others dependencies here with Node.js v14.5.0.

  • send: work fine locally 🚀 ..

send

@dougwilson
Copy link
Contributor

Hi @3imed-jaberi sorry for that! You just stated that you "have checked all the dependencies" and then since the list was incomplete, I was confused on what that meant; if you're still working on that, that's OK, but it wasn't clear that was the case. You may want to clarify if you are still in progress on something in the future :)

@dougwilson
Copy link
Contributor

When you have a completed list, please feel free to ping me at that time so I can review. No need to ping me if it is still in progress :)

@3imed-jaberi
Copy link
Member Author

I told you I have checked all the dependencies and I did already ..

Sorry for my english..., I wanted to let you know that I will be listing some pictures of each dependency. so that the process is clear and you can see that it really works fine locally ..

List (devDeps isn't included) :

  • accepts

accepts

  • array-flatten

array-flatten

  • body-parser

body-parser

  • content-disposition

content-disposition

  • cookie-signature

cookie-signature

  • content-type

content-type

  • debug

debug

  • encodeurl

encodeurl

  • escape-html

escape-html

  • etag

etag

  • findhandler

findhandler

  • fresh

fresh

  • merge-descriptors

merge-descriptors

  • methods

methods

  • on-finished

on-finished

  • parseurl

parseurl

  • path-to-regexp

path-to-regexp

  • range parser

range-parser

  • serve static

serve-static

  • safe-buffer

safe-buffer

  • utils-merge

utils-merge

  • vary

vary

Copy link
Member

@aravindvnair99 aravindvnair99 left a comment

Choose a reason for hiding this comment

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

@3imed-jaberi Can be tested with v14.16 now as that's the latest.

@3imed-jaberi 3imed-jaberi force-pushed the support-nodejs-13.x-and-14.x branch from 723241a to f85780b Compare May 5, 2021 18:01
Copy link
Member Author

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

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

@dougwilson, Node.js have new security releases for 12.x and 10.x. so, should we upgrade to latest ?!

@aravindvnair99
Copy link
Member

@dougwilson, Node.js have new security releases for 12.x and 10.x. so, should we upgrade to latest ?!

@3imed-jaberi I have a PR here with those changes: #4574

@3imed-jaberi
Copy link
Member Author

@dougwilson, Node.js have new security releases for 12.x and 10.x. so, should we upgrade to latest ?!

@3imed-jaberi I have a PR here with those changes: #4574

cool 🚀

@aravindvnair99 aravindvnair99 mentioned this pull request May 14, 2021
@dougwilson dougwilson changed the base branch from 4.18 to master December 16, 2021 02:59
@dougwilson dougwilson force-pushed the support-nodejs-13.x-and-14.x branch from f85780b to cf4cb8d Compare December 16, 2021 03:04
@dougwilson dougwilson force-pushed the support-nodejs-13.x-and-14.x branch from e453fef to 6fe271e Compare December 16, 2021 05:22
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

thank you for all the hard work you did to check the deps

@dougwilson dougwilson merged commit a33266a into expressjs:master Dec 16, 2021
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.

4 participants