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

Replace var with let and const #112

Open
gitdevjin opened this issue Oct 2, 2024 · 8 comments
Open

Replace var with let and const #112

gitdevjin opened this issue Oct 2, 2024 · 8 comments
Labels

Comments

@gitdevjin
Copy link

gitdevjin commented Oct 2, 2024

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
Copy link
Author

gitdevjin commented Oct 2, 2024

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

@sarraf1996
Copy link

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.

@gitdevjin
Copy link
Author

gitdevjin commented Oct 3, 2024

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.

@bjohansebas
Copy link
Member

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.

@sarraf1996
Copy link

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

@IamLizu IamLizu added the future label Oct 3, 2024
@gitdevjin
Copy link
Author

gitdevjin commented Oct 3, 2024

@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
  1. 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.

@sarraf1996
Copy link

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

@SnowMarble
Copy link

SnowMarble commented Oct 16, 2024

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.

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

No branches or pull requests

6 participants
@IamLizu @sarraf1996 @SnowMarble @bjohansebas @gitdevjin and others