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

3ch023 · 2024-12-17T19:14:10Z

when requestUpdate on MAS inline-price or checkout-link is called the 'mas:resolved' event is dispatched twice.
this PR is fixing first event to be 'mas:pending' and it is followed by a single 'mas:resolved' or 'mas:failed'

added nala tests to cover this too.

Test URLs:

aem-code-sync · 2024-12-17T19:14:13Z

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

Re-run PSI checks
Re-sync branch

Commits

13cc5b1 ✅ (latest)
44b4ccc ✅
2d6d64b ✅
ff5fc04 ✅
bf753cb ✅
69b9a58 ✅
f261d64 ✅
4f8b385 ✅
43475cf ✅

aem-code-sync · 2024-12-17T19:14:33Z

	Page	Scores	Audits	Google
📱	/?martech=off
🖥️	/?martech=off

github-actions · 2024-12-17T19:14:41Z

libs/deps/mas/commerce.js

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

github-actions · 2024-12-17T19:14:42Z

libs/deps/mas/mas.js

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

github-actions · 2024-12-17T19:14:42Z

libs/features/mas/dist/mas.js

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

libs/features/mas/src/mas-element.js

codecov · 2024-12-17T19:16:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (c582faa) to head (13cc5b1).

Additional details and impacted files

@@           Coverage Diff           @@
##            stage    #3388   +/-   ##
=======================================
  Coverage   96.45%   96.46%           
=======================================
  Files         255      255           
  Lines       59420    59402   -18     
=======================================
- Hits        57314    57301   -13     
+ Misses       2106     2101    -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yesil · 2024-12-18T07:59:29Z

nala/features/mas/docs/docs.test.js

This is nice 👍

yesil · 2024-12-18T07:59:58Z

nala/features/mas/docs/docs.spec.js

+      name: '@MAS-DOCS-checkout-link',
+      path: '/libs/features/mas/docs/checkout-link.html',
+      browserParams: '?theme=dark',
+      tags: '@mas-docs @checkout-link @commerce @smoke @regression @milo',


do we need mas- prefix for the tags?

i will ask @afmicka
i have a feeling that these tags should be unique for whole milo repo, so if we want an ability to run only our mas docs tests - then it should be @mas-docs, if we want to run 'all' docs - @docs
i am not sure which it should be

yes, those tags are global, and docs would be ambiguous

yesil · 2024-12-18T08:01:12Z

libs/features/mas/src/mas-element.js

@@ -153,6 +153,8 @@ export class MasElement {
        this.state = STATE_FAILED;
        this.update();
        this.log?.error('Failed:', { element: this.wrapperElement, error });
+        // Allow calling code to perform sync updates of this element
+        // before notifying observers about state change


actually, setImmediate is deprecated, should we try it with setTimeout(callback, 0) by this occasion?

i refactored it to just call notify function where we need to call it.. imo this thread breaks are making comprehension harder

yesil · 2024-12-18T08:05:43Z

libs/features/mas/src/mas-element.js

@@ -166,7 +168,8 @@ export class MasElement {
        if (options) this.options = options;


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.

removed it, agree it's an unnessary event

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

github-actions · 2024-12-18T21:39:03Z

libs/deps/mas/merch-card-collection.js

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

github-actions · 2024-12-18T21:39:04Z

libs/deps/mas/merch-sidenav.js

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

github-actions · 2024-12-18T21:39:05Z

libs/deps/mas/merch-subscription-panel.js

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

github-actions · 2024-12-18T21:39:07Z

libs/features/mas/src/constants.js

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

github-actions · 2024-12-19T02:23:34Z

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

yesil · 2024-12-19T09:03:51Z

libs/features/mas/src/inline-price.js

            selectOffers(await promise, options),
            options,
            version,
        );
+        this.masElement.notify();


this logic is handled at mas-element level(*), the custom elements (we may have more in the future) should not know how / when to notify.

(*) https://github.com/adobecom/milo/pull/3388/files#diff-b76179f008b9c485066e722d9e2b9265ccdbfe9d5abb33507d5ab8424e7a45b5L207-L219

got it, i moved it back into mas-element

yesil · 2024-12-19T09:04:09Z

libs/features/mas/src/checkout-link.js

@@ -162,13 +162,15 @@ export class CheckoutLink extends HTMLAnchorElement {
            { ...extraOptions, ...options },
            this,
        );
-        return this.renderOffers(
+        const result = this.renderOffers(


same comment as for inline-price

npeltier · 2024-12-18T08:59:20Z

libs/features/mas/src/mas-element.js

@@ -166,7 +168,8 @@ export class MasElement {
        if (options) this.options = options;
        this.state = STATE_PENDING;
        this.update();
-        setImmediate(() => this.notify());
+        // immediately notify observers about state change
+        this.notify();


couldn't we even remove notification for pending state?

did that, thx

npeltier · 2024-12-19T09:45:56Z

libs/features/mas/src/mas-element.js

@@ -166,7 +168,8 @@ export class MasElement {
        if (options) this.options = options;


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

npeltier · 2024-12-19T09:46:40Z

nala/features/mas/docs/docs.spec.js

+      name: '@MAS-DOCS-checkout-link',
+      path: '/libs/features/mas/docs/checkout-link.html',
+      browserParams: '?theme=dark',
+      tags: '@mas-docs @checkout-link @commerce @smoke @regression @milo',


yes, those tags are global, and docs would be ambiguous

yesil · 2024-12-19T12:00:52Z

libs/features/mas/src/mas-element.js

@@ -148,6 +148,7 @@ export class MasElement {
        if (options) this.options = options;
        this.state = STATE_PENDING;
        this.update();
+        this.log?.debug('Pending:', { element: this.wrapperElement });


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

yesil

nala tests failing?

github-actions · 2024-12-20T01:37:35Z

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.

Roycethan · 2024-12-20T01:49:30Z

Verified, Regressions covered

milo-pr-merge · 2025-01-02T09:16:42Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/commerce.js" overlap. Merging will be attempted in the next batch

milo-pr-merge · 2025-01-07T09:17:23Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/mas.js" overlap. Merging will be attempted in the next batch

Roycethan · 2025-01-07T16:38:00Z

@3ch023 Plz resolve conflicts in this PR to merge, thanks

# Conflicts: # libs/deps/mas/commerce.js # libs/deps/mas/mas.js # libs/deps/mas/merch-card-collection.js # libs/deps/mas/merch-sidenav.js # libs/deps/mas/merch-subscription-panel.js # libs/features/mas/dist/mas.js

milo-pr-merge · 2025-01-09T09:17:27Z

Skipped 3388: "MWPW-161625: fix mas:pending event on requestUpdate" due to file "libs/deps/mas/commerce.js" overlap. Merging will be attempted in the next batch

# Conflicts: # libs/deps/mas/commerce.js # libs/deps/mas/mas.js # libs/features/mas/dist/mas.js

github-actions · 2025-01-09T12:25:10Z

libs/deps/mas/merch-card.js

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

3ch023 added 3 commits December 17, 2024 19:20

fix pendin event notification

43475cf

add nala tests

4f8b385

fix typo

f261d64

3ch023 added the commerce label Dec 17, 2024

github-actions bot reviewed Dec 17, 2024

View reviewed changes

libs/features/mas/src/mas-element.js Show resolved Hide resolved

yesil reviewed Dec 18, 2024

View reviewed changes

address review comments

69b9a58

aem-code-sync bot temporarily deployed to MWPW-161625 December 18, 2024 17:45 Inactive

github-actions bot reviewed Dec 18, 2024

View reviewed changes

yesil reviewed Dec 19, 2024

View reviewed changes

npeltier approved these changes Dec 19, 2024

View reviewed changes

fix review comments

bf753cb

aem-code-sync bot temporarily deployed to MWPW-161625 December 19, 2024 11:19 Inactive

fix

ff5fc04

yesil reviewed Dec 19, 2024

View reviewed changes

yesil approved these changes Dec 19, 2024

View reviewed changes

Roycethan added verified PR has been E2E tested by a reviewer Ready for Stage labels Dec 20, 2024

3ch023 added 2 commits January 8, 2025 10:01

Merge remote-tracking branch 'upstream/stage' into MWPW-161625

2d6d64b

# Conflicts: # libs/deps/mas/commerce.js # libs/deps/mas/mas.js # libs/deps/mas/merch-card-collection.js # libs/deps/mas/merch-sidenav.js # libs/deps/mas/merch-subscription-panel.js # libs/features/mas/dist/mas.js

merge changes

44b4ccc

Merge remote-tracking branch 'upstream/stage' into MWPW-161625

13cc5b1

# Conflicts: # libs/deps/mas/commerce.js # libs/deps/mas/mas.js # libs/features/mas/dist/mas.js

github-actions bot reviewed Jan 9, 2025

View reviewed changes

		@@ -166,7 +168,8 @@ export class MasElement {
		if (options) this.options = options;

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

Are you sure you want to change the base?

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

Conversation

3ch023 commented Dec 17, 2024

aem-code-sync bot commented Dec 17, 2024 • edited Loading

aem-code-sync bot commented Dec 17, 2024 • edited Loading

github-actions bot Dec 17, 2024

Choose a reason for hiding this comment

github-actions bot Dec 17, 2024

Choose a reason for hiding this comment

github-actions bot Dec 17, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3ch023 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot Dec 18, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024

Choose a reason for hiding this comment

3ch023 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesil left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024

Roycethan commented Dec 20, 2024

milo-pr-merge bot commented Jan 2, 2025

milo-pr-merge bot commented Jan 7, 2025

Roycethan commented Jan 7, 2025

milo-pr-merge bot commented Jan 9, 2025

github-actions bot Jan 9, 2025

Choose a reason for hiding this comment

aem-code-sync bot commented Dec 17, 2024 •

edited

Loading

aem-code-sync bot commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

3ch023 Dec 18, 2024 •

edited

Loading

3ch023 Dec 19, 2024 •

edited

Loading