express icon indicating copy to clipboard operation
express copied to clipboard

Dependencies (5.0)

Open vansergen opened this issue 4 years ago • 9 comments

It seems that we can remove a few dependencies before the 5.0 release:

  • methods - We can use the built-in http module:
const { METHODS } = require("http");
const methods = METHODS.map((method) => method.toLowerCase());
  • path-is-absolute - We can use the built-in path module:
const path = require("path");
const isAbsolute = path.isAbsolute("some-path");
  • safe-buffer - We can use the built-in Buffer:
const a = Buffer.from("something");
  • setprototypeof - We can use the Object.setPrototypeOf()
Object.setPrototypeOf(this.request, parent.request);
  • utils-merge - We can use the spread operator:
const opts = { expires: new Date(1), path: '/' , ...options};

vansergen avatar May 16 '20 09:05 vansergen

Good conversation to have! Please don't take this response as a rebuttal, but as the start of a conversation and me thinking out loud.

The methods package essentially is just that snippet, and has zero dependencies of its own. (Although it might be worth revisiting, I don't fully understand the value of the fallback behavior defined in the package for Node versions that don't have http.METHODS).

It exists so that anyone in the ecosystem can use that same standard helper without having to keep that utility code in their own codebase. Having it as a package is essentially the same as if we had that same snippet of code in a utils folder, guarantees that any changes that ever need to happen to it will be synced across every package that uses it. Express maintains that package, as the jshttp org is under the Express project umbrella. For context on a similar choice to use a package vs a oneliner, see this exchange between @dougwilson and I where I learned a little about our choice on these things. Also, in v5 router is being moved into it's own package, which is the system in Express that uses methods the most. Having it as a package ensures that both router, the express tests, and the application system are all using the exact same snippet at all times.

I can't find where path-is-absolute is used in Express, so I don't have anything to say about that, but if it is being used I would agree that's a simple one to replace since it's introduction in Node v0.11.2 👍

safe-buffer can be dropped once Express' lower bound of support is at Node.js v6, I'm still not 100% sure what version v5 will support down to, but that is a good candidate as well once we are there.

Object.setPrototypeOf came to Node in v6.0.0, so again, that can probably be used once version support comes that high.

The spread ... operator came in at Node.js v8.0.0, so same story as the other version support 👍

jonchurch avatar May 16 '20 19:05 jonchurch

See https://github.com/expressjs/express/issues/2755#issuecomment-382499774 for the 5.0 version support: image

xuxucode avatar May 17 '20 16:05 xuxucode

I'm not sure if my comment is being misread here, but it is that we will support down to the LTS at the time, not that we won't support more than that. There has been further discussion since that thread, including in TC meetings, which is what @jonchurch is trying to represent. The version of support we are always aim for is as wide as possible, but there is a balance. Dropping versions for us is more about what we get for it, unrelated to what is supported by the Node.js project. But we will always support at least down to the current LTS version, no exceptions.

dougwilson avatar May 17 '20 16:05 dougwilson

Got it, I misunderstood it before, I thought we just need to support LTS at that time.

xuxucode avatar May 18 '20 00:05 xuxucode

No problem. Also (sorry!) to clarify: it doesn't mean this discussion is over at all; just it will help us to have discussions like this where we specify what the change in Node.js requirement will be for each one (like @jonchurch listed) so we can make decisions on what to do, etc.

dougwilson avatar May 18 '20 00:05 dougwilson

Hi @mgoffan , as this is an issue for the dependencies in 5.0, it is probably not the right place, as Express 5.0 does not have a dependency on path-to-regexp: https://github.com/expressjs/express/blob/5.0/package.json#L29

Edit: It seems @mgoffan deleted their own comment.

dougwilson avatar May 26 '20 19:05 dougwilson

@dougwilson Thanks for the clarification. Just noticed it was here https://github.com/expressjs/express/issues/4171 🙂

mgoffan avatar May 26 '20 20:05 mgoffan

Hi @lucaslago thank you for your comment. If this is an issue with an upstream dependency please report there. If this is something express needs to address, please report though our listed security policy if this is a security issue. I have deleted your post under the assumption it is.

dougwilson avatar Jun 03 '21 23:06 dougwilson

  • setprototypeof: Object.setPrototypeOf (Node 0.11+, )

    Object.setPrototypeOf came to Node in v6.0.0, so again, that can probably be used once version support comes that high.

    MDN says setPrototypeOf was added in Node 0.12, and a quick check with nvm shows that this is indeed true (absent in 0.10, present in 0.11 and 0.12+); not Node 6.

    So the ponyfill is needed in Express 4 as it supports Node 0.10+, but as 5.0.0beta1 already bumped the minimum Node version to 4, it no longer will be.

  • array-flatten: Array.prototype.flat (Node 11+, 2018)

    If the plan is still for Express 5 to only raise the minimum Node version to 4, this would still be necessary.

  • Spread syntax is Node 5 (2015), Object Spread is Node 8.3 (2017)

kisaragi-hiu avatar Sep 23 '23 12:09 kisaragi-hiu