The Wayback Machine - http://web.archive.org/web/20201107150824/https://github.com/expressjs/express/pull/4338
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: Node.js@13.x and Node.js@14.x #4338

Open
wants to merge 2 commits into
base: 4.18
from

Conversation

@3imed-jaberi
Copy link

@3imed-jaberi 3imed-jaberi commented Jul 3, 2020

  • Support Node.js 13.x.
  • Support Node.js 14.x.
@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 3, 2020

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
Author

@3imed-jaberi 3imed-jaberi commented Jul 3, 2020

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
Member

@dougwilson dougwilson commented Jul 3, 2020

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

@3imed-jaberi
Copy link
Author

@3imed-jaberi 3imed-jaberi commented Jul 4, 2020

@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
Member

@dougwilson dougwilson commented Jul 4, 2020

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
Author

@3imed-jaberi 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
Member

@dougwilson dougwilson commented Jul 4, 2020

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
Member

@dougwilson dougwilson commented Jul 4, 2020

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
Author

@3imed-jaberi 3imed-jaberi commented Jul 5, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.