-
Notifications
You must be signed in to change notification settings - Fork 163
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
Create an onReady and onError callback for standalone gnav #2873
base: stage
Are you sure you want to change the base?
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## stage #2873 +/- ##
==========================================
- Coverage 96.24% 96.22% -0.02%
==========================================
Files 237 237
Lines 54219 54243 +24
==========================================
+ Hits 52182 52195 +13
- Misses 2037 2048 +11 ☔ View full report in Codecov by Sentry. |
@@ -317,7 +317,9 @@ class Gnav { | |||
await yieldToMain(); | |||
await task(); | |||
} | |||
|
|||
// Dispatching an event when gnav loads and is visible | |||
window.dispatchEvent(new CustomEvent('feds:nav.ready')); |
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.
window.dispatchEvent(new CustomEvent('feds:nav.ready')); | |
window.dispatchEvent(new CustomEvent('feds:nav:ready')); |
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.
On another note, since it's a milo block, should we dispatch milo:globalnav:ready
? That follows the other eventlisteners such as milo:modal:closed
within the ecosystem
As a more general question, why is that needed and what is the use-case? The ticket and description don't mention anything |
@mokimo This is currently required for the Adobe Home team, but we thought it would be a good use case to support as a standalone component. They want to display a loader state until the Gnav fully loads and be able to handle/track any errors that occur within their app. |
onReady, | ||
onError, |
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.
Since this is an async function couldn't the consumers just do
try {
// do stuff
await loadBlock(...);
// do more stuff
} catch (e) {
// handle error
}
Or they could even do
loadBlock(...).then(onReady).catch(onError)
of course you'd have to await
the initBlock
function inside bootstrapper.js for this to work correctly (
milo/libs/navigation/bootstrapper.js
Line 31 in af28174
initBlock(document.querySelector(targetEl)); |
This would allow us to avoid the rigmarole of callbacks and events that add more complexity to our model of how the global-navigation works. We'd just have to throw irrecoverable errors for the consumer to deal with (you may have to modify loadPostLCP
with a try...catch
to make milo correctly 'consume' global-navigation).
I realize I might be missing some nuance related to what errors should be propagated vs which errors should be handled internally, but my understanding is that the global-navigation already does this adequately; we'd need only to remove the try catch block in the global navigation init function, and modify the consuming end to handle the error (loadPostLCP
in milo's case)(https://github.com/adobecom/milo/blob/af281740d8e910d0e677e3fb556f11c7095aeb50/libs/blocks/global-navigation/global-navigation.js#L1029C1-L1047C4)
Ultimately I think it's better to deal with this problem by propagating exceptions and dealing with exceptions on the consuming end rather than muck around with events and callbacks. Some time back we refused to allow Partner Portals to emit events to signify when specific parts of the global-navigation were loaded #2583 , and I think the same concerns apply here.
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.
If we implement it this way, the consuming client will receive the onError callback only when the gnav content is not found.
All other errors, such as IMS issues or errors loading fragments, would be logged internally. To achieve this we could add a console.error line here
Thoughts? @narcis-radu @mokimo @salonijain3
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. |
41ec6c5
to
35bebf5
Compare
@narcis-radu @mokimo Could you please review the updated changes? |
Resolves: MWPW-158168
Test URLs:
Qa: https://adobecom.github.io/nav-consumer/navigation.html?authoringpath=/federal/dev&env=stage&navbranch=gnav-event