Skip to content
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

proposal: preserve fetching order #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion examples/case_studies/contacts/index.xml.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ module.exports = function handler(req, res, next) {
const { query } = urlParse(req.originalUrl, true);

// no search or next page? pass through to 11ty to render the entire document
if (query.search === undefined && !query.page) {
// note: we also test for "template" to work mitigate this bug
// https://github.com/Instawork/hyperview/issues/735
if (query.search === undefined && !query.page && !query.template) {
next();
return;
}
Expand Down
2 changes: 2 additions & 0 deletions examples/case_studies/contacts/list.xml.njk
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
action="replace"
target="contacts"
href="?template=list"
debounce="200"
preserve-order="true"
show-during-load="loading-indicator"
/>
</text-field>
Expand Down
59 changes: 42 additions & 17 deletions src/core/components/hv-screen/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -505,27 +505,52 @@ export default class HvScreen extends React.Component {
doc: newRoot,
});

// If a target is specified and exists, use it. Otherwise, the action target defaults
// to the element triggering the action.
const getTargetElement = () => {
let targetElement = targetId ? this.doc?.getElementById(targetId) : currentElement;
if (!targetElement) {
targetElement = currentElement;
}
return targetElement
}

// Before fetching, check if the behavior needs order to be preserved.
// If so, we need to set a flag on the element that we check upon receiving the response.
// If that flag has changed, that means another more recent request has been made, and we
// need to discard the response.
const targetElement = getTargetElement();
const preserveOrderId = (
behaviorElement || currentElement
)?.getAttribute('preserve-order') === 'true'
? String(Math.random())
Comment on lines +523 to +526
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Use optional chaining (?.) consistently here

: null;
if (preserveOrderId !== null) {
targetElement.setAttribute('_preserve-order-id', preserveOrderId);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Use a constant for '_preserve-order-id' to avoid magic strings

}
Comment on lines +522 to +530
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using a more descriptive name for preserveOrderId, such as requestSequenceId


// Fetch the resource, then perform the action on the target and undo indicators.
const fetchAndUpdate = () => this.fetchElement(href, verb, newRoot, formData)
.then((newElement) => {
// If a target is specified and exists, use it. Otherwise, the action target defaults
// to the element triggering the action.
let targetElement = targetId ? this.doc?.getElementById(targetId) : currentElement;
if (!targetElement) {
targetElement = currentElement;
}

if (newElement) {
newRoot = Behaviors.performUpdate(action, targetElement, newElement);
} else {
// 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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Add a comment explaining the purpose of shouldSkipUpdate for better code readability

if (!shouldSkipUpdate) {
if (newElement) {
newRoot = Behaviors.performUpdate(action, targetElement, newElement);
if (preserveOrderId) {
// Un-set the preserve order id on the target element as the update is now done
targetElement.setAttribute('_preserve-order-id', "");
}
Comment on lines +540 to +543
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider moving this logic to a separate method for better organization

} else {
// When fetch fails, make sure to get the latest version of the doc to avoid any race conditions
newRoot = this.doc;
}
newRoot = Behaviors.setIndicatorsAfterLoad(showIndicatorIdList, hideIndicatorIdList, newRoot);
// Re-render the modifications
this.setState({
doc: newRoot,
});
}
newRoot = Behaviors.setIndicatorsAfterLoad(showIndicatorIdList, hideIndicatorIdList, newRoot);
// Re-render the modifications
this.setState({
doc: newRoot,
});

if (typeof onEnd === 'function') {
onEnd();
Expand Down