-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Declare SearchRequest state variables in constructor. #20578
Declare SearchRequest state variables in constructor. #20578
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.
code changes lgtm (did not pull down and test).
return false; | ||
} | ||
|
||
if (this.stopped) { |
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 don't suppose in your travels of courier you understand the logic behind this? I like how you split this out, but it logically seems strange to me. No worries if you don't since this PR is unrelated, but if you do know, I think this could go for a comment. Why can't this be started if it's stopped? you can't "restart"? And below, I guess this._isFetchRequested
means that it's already been started? The top one is obvious.
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.
Funny you should ask! I made these comments as I started trying to make sense of all of this but then I gave up. A lot of this logic exists to support courier's indirection-based architecture and from everything I've seen, I think the best solution will be to replace it with a more explicit architecture (e.g. #20364).
Why can't this be started if it's stopped? you can't "restart"?
I don't know the answers to these ones.
And below, I guess this._isFetchRequested means that it's already been started?
I actually read a comment you left in this file which explained this for me:
/**
* Called by the fetchSoon module when this request has been sent to
* be fetched. At that point the request is somewhere between `ready-to-start`
* and `started`. The fetch module then waits a short period of time to
* allow requests to build up in the request queue, and then immediately
* fetches all requests that return true from `isFetchRequestedAndPending()`
*
* @return {undefined}
*/
_setFetchRequested() {
this._isFetchRequested = true;
}
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.
So I guess it means it's going to start, but hasn't started yet?
💚 Build Succeeded |
No description provided.