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

[core] Allow for..of loops #42600

Merged
merged 1 commit into from
Jun 11, 2024
Merged

[core] Allow for..of loops #42600

merged 1 commit into from
Jun 11, 2024

Conversation

michaldudak
Copy link
Member

Removed the eslint-config-airbnb-base package. It is meant to be used in non-React apps.
The primary reason for its removal is to allow using iterators and the for..of loops, as the browser support is good (the spec is a part of ES2015).

Removal of this package also removes 3 additional no-restricted-syntax rules:

{
  selector: 'ForInStatement',
  message: 'for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array.',
},
{
  selector: 'LabeledStatement',
  message: 'Labels are a form of GOTO; using them makes code confusing and hard to maintain and understand.',
},
{
  selector: 'WithStatement',
  message: '`with` is disallowed in strict mode because it makes code impossible to predict and optimize.',
},

However, we have other rules that prevent writing unsafe code using these constructs: guard-for-in, no-labels, and no-with.

Note that writing for..of loops in projects in this repo is still prevented by tsconfig's target being set to "ES5". Changing this should be discussed first in a separate PR.

@michaldudak michaldudak added the scope: code-infra Specific to the core-infra product label Jun 10, 2024
@michaldudak michaldudak requested a review from a team June 10, 2024 09:41
@mui-bot
Copy link

mui-bot commented Jun 10, 2024

Netlify deploy preview

https://deploy-preview-42600--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 674d363

@@ -65,7 +65,7 @@ function createTheme(options = {}, ...args) {
const traverse = (node, component) => {
let key;

// eslint-disable-next-line guard-for-in, no-restricted-syntax
// eslint-disable-next-line guard-for-in
for (key in node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily part of this PR, but is this in intentional? Is it required to traverse prototype properties here?

Another thing I noticed while sniffing around this code is that we can likely easily optimize occurences of str.indexOf(search) === 0 to the more performant string.startsWith(search)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering https://github.com/mui/material-ui/pull/13919/files#r241988655, this code may benefit from a more thorough review (cc @DiegoAndai) ;)
I'd like to keep this PR focused on eslint changes only, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Janpot may I ask you to create an issue for the optimization? that way we can ask the community for contributions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll verify first whether it's a real optimization

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment with possible side-quests 🙂

@michaldudak michaldudak merged commit 7a98d61 into mui:next Jun 11, 2024
23 checks passed
@michaldudak michaldudak deleted the allow-for-of branch June 11, 2024 08:11
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jun 12, 2024
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants