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.

Sign up for GitHub

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

MWPW-161625: fix mas:pending event on requestUpdate #3388

Open
wants to merge 9 commits into
base: stage
Choose a base branch
from
+180 −161
Open

MWPW-161625: fix mas:pending event on requestUpdate #3388

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
43475cf
fix pendin event notification
3ch023 Dec 17, 2024
4f8b385
add nala tests
3ch023 Dec 17, 2024
f261d64
fix typo
3ch023 Dec 17, 2024
69b9a58
address review comments
3ch023 Dec 18, 2024
bf753cb
fix review comments
3ch023 Dec 19, 2024
ff5fc04
fix
3ch023 Dec 19, 2024
2d6d64b
Merge remote-tracking branch 'upstream/stage' into MWPW-161625
3ch023 Jan 8, 2025
44b4ccc
merge changes
3ch023 Jan 8, 2025
13cc5b1
Merge remote-tracking branch 'upstream/stage' into MWPW-161625
3ch023 Jan 9, 2025
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
2 changes: 1 addition & 1 deletion libs/deps/mas/commerce.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] reported by reviewdog 🐶
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion libs/deps/mas/mas.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] reported by reviewdog 🐶
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion libs/features/mas/dist/mas.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] reported by reviewdog 🐶
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions libs/features/mas/src/checkout-link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,13 @@ export class CheckoutLink extends HTMLAnchorElement {
{ ...extraOptions, ...options },
this,
);
const result = this.renderOffers(
return this.renderOffers(
offers.flat(),
options,
{},
checkoutAction,
version,
);
this.masElement.notify();
return result;
}

/**
Expand Down
4 changes: 1 addition & 3 deletions libs/features/mas/src/inline-price.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,11 @@ export class InlinePrice extends HTMLSpanElement {
const version = this.masElement.togglePending(options);
this.innerHTML = '';
const [promise] = service.resolveOfferSelectors(options);
const result = this.renderOffers(
return this.renderOffers(
selectOffers(await promise, options),
options,
version,
);
this.masElement.notify();
return result;
}

// TODO: can be extended to accept array of offers and compute subtotal price
Expand Down
6 changes: 4 additions & 2 deletions libs/features/mas/src/mas-element.js
View file
Open in desktop
3ch023 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export class MasElement {
if (options) this.options = options;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you can listen the very first mas:pending event.
If you try to register an even listener for a <span is="price" data-wcs-osi="xyz">/span>, it is possible that, the pending event will be dispatched before you register.
To check. I would really remove pending all together, and consider the default state as pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it, agree it's an unnessary event

Copy link
Contributor

Choose a reason for hiding this comment

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

i would somewhere here though add some debug log to mention that element is pending resolution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added, thx

this.state = STATE_PENDING;
this.update();
this.log?.debug('Pending:', { element: this.wrapperElement });
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this has any performance impact, this it needs to kind of serialize the wrapperElement.

return this.version;
}

Expand Down Expand Up @@ -195,10 +196,11 @@ export class MasElement {
this.state = state;
this.error = error;
this.value = value;
// Update CSS and notify observers/listeners
// Update CSS
this.update();
this.notify();
}
// notify observers/listeners
this.notify();
} catch (error) {
log.error(`Failed to render mas-element: `, error);
this.toggleFailed(this.version, error, options);
Expand Down
Loading