cookie-parser icon indicating copy to clipboard operation
cookie-parser copied to clipboard

Replace `var` with `let` and `const`

Open gitdevjin opened this issue 1 year ago • 9 comments

Description

According to the docs, using var is not recommended, unless it's necessary, It would be useful to replace var in index.js with let or const depending on their scope and usage. And also as it's less error-prone, this change can make the code more robust for the possible future code change.

Task

  • Replace var with let or const in index.js file
  • Updatevar with let or const in README.md file

Supporting Docs and Articles

w3schools variables Why don’t we use var anymore?

gitdevjin avatar Oct 02 '24 19:10 gitdevjin

I would love to work on this. Can I make a PR for this? :)

gitdevjin avatar Oct 02 '24 19:10 gitdevjin

Hi @gitdevjin

First of all, I am sharing my opinion on the basis of my understanding.

I have gone though package.json file of the latest version of this repository and found that package.json specifies "engines": { "node": ">= 0.8.0" } within it.

It indicates that the application is compatible with Node.js version 0.8.0 and later. However, if you replace var with let and const, you need to consider the following:

Node.js Version Compatibility:

let and const were introduced in ECMAScript 2015 (ES6): https://hacks.mozilla.org/2015/07/es6-in-depth-let-and-const. Also, the source code in this repository does not utilize ES6 features such as let, const, arrow functions, or other modern JavaScript enhancements. If this application runs in environments with older Node.js versions (>= 0.8.0) that do not support ES6 features, using let and const will cause syntax errors and as a result build will be unsuccessful.

You can go through the below website that provides a comprehensive compatibility table for ECMAScript (JavaScript) features across various versions of Node.js. It allows developers to check which ECMAScript features are supported in different Node.js versions. https://node.green

I would like to reiterate that the information I've provided reflects my personal view. I encourage members of the express team and others to share their perspectives and insights, as they may have different interpretations or findings.

sarraf1996 avatar Oct 02 '24 23:10 sarraf1996

Hi @sarraf1996. Thank you so much for your feedback. I realize now that I didn’t fully consider the different environments where the project might be used. My intention was to transition the code to a newer syntax, but I didn’t think carefully enough about compatibility with older Node.js versions. I’ve learned a lot from your comment, and I really appreciate the insight. It would be great to hear from other members of the Express team about whether dropping support for older Node.js versions might be an option in the future. That said, my question about dropping the support is purely out of curiosity, and I understand that I may need to close my pull request.

gitdevjin avatar Oct 03 '24 00:10 gitdevjin

I don't think there's a problem with changing var to const or let, as, for example, multer and version 4 of express used them while still supporting very old versions of Node.js.

Also, the discussion about dropping support for old Node.js versions has been ongoing; body-parser has already stopped supporting old Node.js versions, and response-time is also planning to drop support for old Node.js versions.

bjohansebas avatar Oct 03 '24 02:10 bjohansebas

@bjohansebas Thanks for your inputs.

I tried to run 2 simple statements below. const id = 'test101'; let age = 30;

First, I tried to run in Node v0.8.0 (minimum compatible engine version of current repo. state) where const didn't give any syntax error but let gave an error SyntaxError: Unexpected identifier.

Then, I tried to run the same code in other version of Node v20.15.0 installed on my system and in that version, both const and let worked without any issues.

Please refer the screenshots for both the attempts. Node_v0 8 0 Node_v20 15 0

If we take Node v0.8.0 into consideration, then let is not supported. So, if var is changed to let, will it not throw the syntax error as stated in the screenshot above?

Your valuable inputs would be much appreciated.

sarraf1996 avatar Oct 03 '24 02:10 sarraf1996

@sarraf1996 I have tested the let and const tokens with various Node.js versions. Without using 'use strict', they support up to v6.17.1. However, with 'use strict' declared at the top of the file, compatibility extends back to v4.0.0. I'm curious if it would be acceptable to drop support for versions older than v4.0.0 or if doing so could cause significant issues, but I want to emphasize that I'm not forcing the project to transition in any particular direction; I am just genuinely curious.

  1. Testing v 6.17.1 wihtout use strict Screenshot 2024-10-03 at 1 49 19 AM

  2. Testing v 4.0.0 with use strict Screenshot 2024-10-03 at 1 57 00 AM

Thank you all, @sarraf1996 @bjohansebas, for your valuable insights! I’m just a student learning programming, and I’m eager to participate in open-source development. I’m learning so much from your feedback, and I truly appreciate your support.

gitdevjin avatar Oct 03 '24 06:10 gitdevjin

@gitdevjin Thank you for your extended checking with and without use strict directive which I believe I missed to check :). I really appreciate your effort and I do agree with your results and thoughts.

As previously mentioned by @bjohansebas, the discussion around dropping support for older Node.js versions has been ongoing. Your issue and PR have already been marked with the future label, indicating that this topic is under consideration. We'll keep an eye on the decisions made by the team as the discussion progresses.

sarraf1996 avatar Oct 03 '24 11:10 sarraf1996

express has already released v5, which dropped support for Nodejs before 18. https://expressjs.com/2024/10/15/v5-release.html#goodbye-nodejs-010-hello-node-18 I think we might be able to consider moving on to a new major version with minimal node 18 support.

But cookie-parser is a very lightweight library and has been working well for years. So, I think keeping var in the code is not that a big deal while supporting old version of nodejs.

SnowMarble avatar Oct 16 '24 23:10 SnowMarble

FWIW, dropping support for Node.js versions below 6 also would allow cookie and cookie-signature dependencies to be bumped to latest.

I think that can be a nice middle-ground between status-quo and going all the way to 18?

3nprob avatar Sep 28 '25 03:09 3nprob