-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(uses-preload): prevent infinite loop #5184
Conversation
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.
nice. thx for these fixes.
one idea
* @return {boolean} | ||
*/ | ||
static hasCycle(node) { | ||
static hasCycle(node, direction = 'all') { | ||
if (direction === 'all') { |
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.
it seems like the only use we have of this method checks things with the root node, implying we only care about one direction. Maybe we just rename this to hasCycleInDependents until we need the other direction?
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.
wait, isn't it always checking both? Why have an option at all
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.
discussed in person, I'll add a comment here
const nodesToExplore = direction === 'dependents' ? | ||
currentNode._dependents : | ||
currentNode._dependencies; | ||
for (const dependent of nodesToExplore) { |
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.
call it nextNode
or something? Confusing to flatten like that
* @return {boolean} | ||
*/ | ||
static hasCycle(node) { | ||
static hasCycle(node, direction = 'all') { | ||
if (direction === 'all') { |
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.
wait, isn't it always checking both? Why have an option at all
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.
LGTM!
fixes an infinite loop on https://emojityper.com/
attacked from multiple prongs: