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

chore: updated the eslint rule to disable no-restricted-syntax #1491

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aryamohanan
Copy link
Contributor

@aryamohanan aryamohanan commented Dec 18, 2024

Our team follows the Airbnb JavaScript style guide, and no-restricted-syntax rule recommends avoiding constructs like for..in, for..of, labeled statements, I believe that these constructs can be practical in some cases for our project. Specifically, we are comfortable using for loops when needed.

Since we’ve already ignored this rule in 15 places across our codebase, disabling it globally would simplify our ESLint configuration and eliminate the need for repetitive exclusions. This change allows us to focus on more relevant rules for our code while maintaining the flexibility to use for loops and similar constructs where appropriate and avoid adding unwanted comments.

I think it’s reasonable to disable this rule for our team.

@aryamohanan aryamohanan marked this pull request as ready for review December 18, 2024 06:28
@aryamohanan aryamohanan requested a review from a team as a code owner December 18, 2024 06:28
@kirrg001
Copy link
Contributor

Why was this rule off in the first place?

@aryamohanan
Copy link
Contributor Author

Why was this rule off in the first place?

The rule was turned off to make things easier for our code base. Constructs like for..in and for..of can be useful in some cases, even though the Airbnb guide discourages them. Since we’ve already ignored this rule in several places, it makes sense to turn it off globally.

This change simplifies our ESLint setup, saves time on repetitive exclusions, and lets us focus on more relevant rules for our codebase. If we find this rule beneficial in the future, we can always re-enable it.

@abhilash-sivan
Copy link
Contributor

IMO wouldn't it be simpler to keep it as is? This way, anyone coding will understand that we generally avoid using loop syntax, but we can override it when absolutely necessary.

@kirrg001
Copy link
Contributor

even though the Airbnb guide discourages them.

Any idea why?

@aryamohanan
Copy link
Contributor Author

even though the Airbnb guide discourages them.

Any idea why?

The Airbnb style guide discourages constructs like for..in and for..of mainly to avoid potential issues like unintended side effects or bugs (for example, for..in can iterate over inherited properties). They also encourage using higher-level methods like map, filter, or reduce for better readability and maintainability. They added this rule in 2016.
reference https://github.com/airbnb/javascript?tab=readme-ov-file#iterators-and-generators
There are simliar discussions airbnb/javascript#851

IMO wouldn't it be simpler to keep it as is? This way, anyone coding will understand that we generally avoid using loop syntax, but we can override it when absolutely necessary.

I see your point. Keeping it as is would indeed make the rule clear to everyone. However, if we end up repeatedly overriding the rule, it might create unnecessary clutter in the codebase. Disabling it globally could simplify things, but I understand the value in leaving it in place for clarity. We could always revisit it later if we feel the need of this rule.

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

Successfully merging this pull request may close these issues.

3 participants