Skip to content

Conversation

@tinfoil-knight
Copy link
Contributor

@tinfoil-knight tinfoil-knight commented Jul 6, 2021

- Summary

See #1891

Enables the fp/no-loops for eslint-plugin-fp.

  • for loops were changed to use forEach or other array methods like map, some or every wherever appropriate.
  • Retry logic was changed from using loop to being recursive in nature.
  • 4 instances of for loop were ignored due to any of the following reasons:
    • async/await doesn't work with forEach
    • recursive solutions would be functional but hard to read/understand
    • Promise.allSettled is not supported below 12.9.0 (Can replace making await() requests in for loops)

- Test plan

  • npx ava --serial --verbose ran without any errors.

@tinfoil-knight tinfoil-knight requested a review from a team as a code owner July 6, 2021 11:23
@erezrokah erezrokah added the type: chore work needed to keep the product and development running smoothly label Jul 6, 2021
@tinfoil-knight
Copy link
Contributor Author

All failing tests have been fixed.

@tinfoil-knight
Copy link
Contributor Author

tinfoil-knight commented Jul 6, 2021

It seems that the ?. operator isn't supported yet for node in the CI. Will need to change it.

@erezrokah What version of node is the CI supporting for tests?

Also, I received a ESLint warning when using Promise.allSettled which is incompatible with current node engine specified but didn't get one for using the optional chaining operator which is only supported for >=14.0.0.

@tinfoil-knight
Copy link
Contributor Author

tinfoil-knight commented Jul 7, 2021

@erezrokah All tests are passing for the CI except on ubuntu-latest, bash, 10.x.

The error seems to be in the detect-server.js file. I can't reproduce the issue but I'll try pushing a fix.

@erezrokah
Copy link
Contributor

@erezrokah What version of node is the CI supporting for tests?

The CLI supports Node.js >= 10.18.0, The optional chaining operator ?. is supported for Node.js >= 14

@erezrokah
Copy link
Contributor

The error seems to be in the detect-server.js file. I can't reproduce the issue but I'll try pushing a fix.

You can use nvm to install and switch to Node.js 10 locally.

Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

I've added a few more comments.

In general we should treat fp/no-loops as a guideline and not a strict rule, as sometimes it's better to have a for loop.

@tinfoil-knight
Copy link
Contributor Author

@erezrokah I've made all the requested changes and tested on Node.js 10 w/ npx ava -v -s

@tinfoil-knight tinfoil-knight force-pushed the 1891-no-loops branch 2 times, most recently from 1babb95 to 2a2daa1 Compare July 7, 2021 19:32
@tinfoil-knight tinfoil-knight requested a review from erezrokah July 9, 2021 18:14
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

🚀

@erezrokah erezrokah enabled auto-merge (squash) July 12, 2021 15:54
@erezrokah erezrokah disabled auto-merge July 12, 2021 16:25
@erezrokah erezrokah enabled auto-merge (squash) July 12, 2021 16:25
@erezrokah erezrokah disabled auto-merge July 12, 2021 16:53
@erezrokah erezrokah merged commit aa6c8fb into netlify:main Jul 12, 2021
@tinfoil-knight
Copy link
Contributor Author

Thank you @erezrokah for merging my first PR in this repository. More PRs upcoming!

@erezrokah
Copy link
Contributor

Great work @tinfoil-knight, looking forward 👍

@tinfoil-knight tinfoil-knight deleted the 1891-no-loops branch September 24, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: chore work needed to keep the product and development running smoothly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants