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
♻️ Refactor: added defaultmap + locking mechanism for intercepts queue #1441
♻️ Refactor: added defaultmap + locking mechanism for intercepts queue #1441
Changes from 3 commits
3a81d2d
2a12528
90b6085
1cda91c
63ec621
8fb51fc
f70b9ea
c1ec3d6
98ff139
f3c6511
ebd0542
924a32f
3582916
e617894
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
when is something deleted from this map ?
also considering we have this, we dont need any state sets right ?
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.
Nothing is ever deleted from
#requestsLifeCycleHandler
since it is only used to manage lifecycles.We can ideally delete
requestId
once all the locks are released from it's object, ie. it has reached end of lifecycle.But, I don't see the point of it, as this is a very light map.
Need
#pending
to communicate fromrequestWillBeSent
torequestPaused
states.Can remove
#intercepts
completely.Need
#requests
as it keeps mapping of all active requests and is whatidle
depends on.Need
#authentication
as this keeps track of all requests that have already been authenticated.Need
#aborted
to keep track of aborted requests.In the current PR, I am planning to only remove
#intercepts
queue as it was used for communicating fromrequestPaused
torequestWillBeSent
but that should never happen now.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.
as this is a very light map.
do not fully agree with this as we could have thousands of requests in a single page load [ in fact we do for many storybook builds ] [ even though not blocking as it would be still few kbs max ]_requestPaused
couldawait requestWillBeSent
, so it doesnt need to read from#pending
as far as i understand. Because we are also storing event in pending with request id, you can justresolve
withevent
for request will be sent#requests
agree we need it for now at least#authentication
could be done by checking if auth promise is already resolved [ but i dont mind using set either ]#aborted
is not necessary as you could just setaborted
on#request
object [ or in lifecycle like lifecyle.aborted ] it was done the way it is because we could processaborted
event beforerequestWillBeSent
and it wont exist in requests at all but asloadingFailed
would await onrequestWillBeSent
we can be sure that request already exists in requestsAlso other refactors are optional, not necessary in current pr
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.
lets remove it entirely before merge