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

Use strict equality operator over equality operator #751

Open
prajwalkulkarni opened this issue Jan 19, 2024 · 5 comments
Open

Use strict equality operator over equality operator #751

prajwalkulkarni opened this issue Jan 19, 2024 · 5 comments

Comments

@prajwalkulkarni
Copy link
Contributor

Description
The present codebase of this library uses regular equality checks to strict equality checks which is not a good practice. == does shallow comparison ignoring the type of the operands being compared, which could lead to unexpected bugs.

A Few basic rules could be enforced that would ensure best practices are followed across the source code which could be done by configuring eslint's rules to include the strict equality check.
However, enforcing this rule across the codebase at once would be overwhelming to review. Hence, I feel this could be done in phases, starting with js files as they're lesser in number, and then subsequently making changes to the typescript files.

@NicolasCARPi
Copy link
Contributor

I agree, but I would start with TS files.

@prajwalkulkarni
Copy link
Contributor Author

Great. I could start working on TS files, but I feel it could be done module-wise (parsers/WebGL and rest of the TS files) instead of making all the changes at once(reason: it'd be difficult for the maintainer to review lots of file changes at once, even though the changes are small), please let me know otherwise.

@NicolasCARPi
Copy link
Contributor

Shorter PR are better yes, and hopefully this change is without impact. And if there is an impact it means there is a bug. So a big PR with only the same exact change on all files make sense. I think you can even simply add the rule in eslint and run it with --fix just for this rule and you're done!

@prajwalkulkarni
Copy link
Contributor Author

Yes, hopefully, it is not a breaking change. But, I fear if anywhere in the code if any identifier/value was compared against null or undefined. I believe running tests should be sufficient to verify this. I will get back in sometime.

@prajwalkulkarni
Copy link
Contributor Author

So I updated all equality operators to be strict equality checks and as I feared the tests started failing, since there were many file changes it was getting difficult to iron out the bug even when the error pointed out to certain file(s), hence I had to revert the changes, and now, I've made changes only at those places where it is absolutely fine to do so (more info in the PR #754 ). And, at places where null and undefined were compared, I believe making changes at those places is what would've likely caused the tests to fail. Therefore, I used the following rule in eslint while updating the operators.
"eqeqeq" : ["error", "always",{"null":"ignore"}],, and I did a quick self-review and further updated the operators where the types of operands were known and same.

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

No branches or pull requests

2 participants