Replace `var` with `let` and `const`
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
varwithletorconstinindex.jsfile - Update
varwithletorconstinREADME.mdfile
Supporting Docs and Articles
I would love to work on this. Can I make a PR for this? :)
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.
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.
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 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.
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
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.
-
Testing
v 6.17.1wihtoutuse strict -
Testing
v 4.0.0withuse strict
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 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.
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.
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?