Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Start using eslint-plugin-deprecation to find deprecated code #3275
Start using eslint-plugin-deprecation to find deprecated code #3275
Changes from 6 commits
a3f9518
deab3e6
dd9ca80
8397f58
d5932fc
12a3af0
70461d8
e126dfb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since array.pop() returns a number OR undefined I needed to add an alternative for the undefined case. This is a best effort for a random number bigger than 1000 without using a library to create it. This case should never happen since the uniqueNumbers array is never empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of having code that's not intended to execute, and that branch isn't just throwing an error. I have a few potential solutions to make things a little cleaner:
Throw an error instead
I'd be happy if we threw an error if
pop
ever returnsundefined
. As you say it shouldn't ever happen, so if it does then the code needs to be updated rather than using a fallback strategy.Iterate over
uniqueNumbers
Instead of doing
Array.from({ length: 1000 }, (_, i) =>
to initiate the loop, douniqueNumbers.map(repoId =>
so that we remove the need to callpop
.Just use
Math.random()
and don't precompute a random listThe repo ids don't have to go from 0-999. They just need to be unique, and even then it's only the storybook so not too critical. If we use big enough numbers, e.g.
Math.floor(Math.random() * 1000000000000)
(AFAIK the max safe int in javascript is9007199254740991
), then the chance of getting duplicates in our 1000 repo ids becomes very low.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer the error, as you say it should never happen and if it does we should look at it!