proposal: preserve fetching order #2
+47
−18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR introduce a new behavior boolean attribute, `preserve-order`. This attribute is used by behaviors that "fetch" content, i.e. anytime there's a `href` attribute set, with an update action such as `replace`, `replace-inner`, etc. When present, we compute an unique id before the request is emitted, and store it on an attribute of the target element. When the response arrives, we read the attribute of the target element and compare it to the one we generated before the request was sent. If the attribute is set with a value that's different from the one set before the request, we assume this means another request/response cycle took place, and the response we're presently dealing with is "outdated", so we discard it.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a new 'preserve-order' attribute to handle sequenced requests in Hyperview, addressing potential race conditions in asynchronous content fetching.
- Added 'preserve-order' attribute to the search behavior in
examples/case_studies/contacts/list.xml.njk - Implemented logic in
src/core/components/hv-screen/index.jsto track and validate request order using unique IDs - Modified
examples/case_studies/contacts/index.xml.jsto include an additional condition for query parameter checks - Refactored
onUpdateFragmentmethod in HvScreen component for improved code organization
3 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
Comment on lines
+522
to
+530
| const targetElement = getTargetElement(); | ||
| const preserveOrderId = ( | ||
| behaviorElement || currentElement | ||
| )?.getAttribute('preserve-order') === 'true' | ||
| ? String(Math.random()) | ||
| : null; | ||
| if (preserveOrderId !== null) { | ||
| targetElement.setAttribute('_preserve-order-id', preserveOrderId); | ||
| } |
There was a problem hiding this comment.
style: Consider using a more descriptive name for preserveOrderId, such as requestSequenceId
| // When fetch fails, make sure to get the latest version of the doc to avoid any race conditions | ||
| newRoot = this.doc; | ||
| const targetElement = getTargetElement(); | ||
| const shouldSkipUpdate = preserveOrderId !== null && targetElement.getAttribute('_preserve-order-id') !== preserveOrderId; |
There was a problem hiding this comment.
style: Add a comment explaining the purpose of shouldSkipUpdate for better code readability
Comment on lines
+540
to
+543
| if (preserveOrderId) { | ||
| // Un-set the preserve order id on the target element as the update is now done | ||
| targetElement.setAttribute('_preserve-order-id', ""); | ||
| } |
There was a problem hiding this comment.
style: Consider moving this logic to a separate method for better organization
| ? String(Math.random()) | ||
| : null; | ||
| if (preserveOrderId !== null) { | ||
| targetElement.setAttribute('_preserve-order-id', preserveOrderId); |
There was a problem hiding this comment.
style: Use a constant for '_preserve-order-id' to avoid magic strings
Comment on lines
+523
to
+526
| const preserveOrderId = ( | ||
| behaviorElement || currentElement | ||
| )?.getAttribute('preserve-order') === 'true' | ||
| ? String(Math.random()) |
There was a problem hiding this comment.
style: Use optional chaining (?.) consistently here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR introduce a new behavior boolean attribute,
preserve-order. This attribute is used by behaviors that "fetch" content, i.e. anytime there's ahrefattribute set, with an update action such asreplace,replace-inner, etc. When present, we compute a unique id before the request is emitted, and store it on an attribute of the target element. When the response arrives, we read the attribute of the target element and compare it to the one we generated before the request was sent. If the attribute is set with a value that's different from the one set before the request, we assume this means another request/response cycle took place, and the response we're presently dealing with is "outdated", so we discard it.To be discussed: