Skip to content

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 13, 2025

@humitos humitos force-pushed the humitos/lit-context-config branch from 952d5d5 to 2289537 Compare January 13, 2025 15:23
@humitos
Copy link
Member Author

humitos commented Jan 13, 2025

I merged #455 into this PR to give it a try, and I was able to re-display a notification by clicking on the "Change URL" button 🎉 . It seems this Lit Context pattern is proving that's going to work for what we want to do 👍🏼 .

There is going to be some updates we will need to do to our addons, tho. Example: I found that this.autoDismissed reactive property won't work immediately because the render() method returns nothing immediately if it was auto dismissed. That particular example might not be an issue, but it's something to consider.

@humitos humitos self-assigned this Jan 15, 2025
@agjohnson
Copy link
Contributor

I pushed up an example of using ContextRoot at #499

agjohnson and others added 5 commits January 16, 2025 12:23
Additional elements shouldn't be needed in our structure. This is how we
can use ContextRoot with `document.html` as the context root for
providers/consumers of the config context.

Both the flyout and the notifications load for me with this change.
@humitos
Copy link
Member Author

humitos commented Jan 16, 2025

@agjohnson I updated this PR to follow a pattern:

  • migrate addons to be LitElement (eg. Analytics, CustomScript)
  • use _config as ContextConsumer
  • always validate the _config on render() before using it
  • I added TODOs to always initialize the addons, remove the config from initialization and move the httpStatus check inside the render() method

Can you provide an early review here taking into account the code written and the points I highlighted here? I understand we are moving in the right direction, tho.

@agjohnson
Copy link
Contributor

Yeah you look on a good path there, I think everything you've done so far makes sense 👍

@humitos humitos mentioned this pull request Jan 22, 2025
@humitos
Copy link
Member Author

humitos commented Jan 22, 2025

I've been thinking a little more about this and I think we need something different here. The current approach this PR is following is re-render the addon every time the configContext changes (it changes when the APIv3 data is ready at initialization or due to a history change. This pattern has some downsides:

  • Requires migrating all addons to LitElement
  • The check for isEnabled inside the addon instance after initializing it.
  • The isEnabled check is done every time the addon calls render() (e.g. click on Flyout to expand/collapse)
  • Instances of addons are created even if the addon is disabled, calling connectedCallback when it shouldn't.
  • Can't use the HTTP status to decide whether o not enable the addon.

Instead, I think we can create a AddonsApp that listen to the ContextRoot and manages all the addons. Basically, doing what we are currently doing on index.js. This app will do:

  • The first time it will initialize all the addons
  • On following calls, it will update the addons by calling loadConfig() on each of them with the updated config object received from the ContextRoot

I understand this looks a lot simpler to implement and follows the pattern we currently have. I will give it a try and see where I end.

@humitos
Copy link
Member Author

humitos commented Jan 22, 2025

I got everything working in #504 with 10% of the effort. I'm going to close this PR. We can continue there.

@humitos humitos closed this Jan 22, 2025
humitos added a commit that referenced this pull request Feb 11, 2025
This PR follows the approach exposed in
#491 (comment).
Basically, it manage all the `ContextRoot` logic in one place and update
all the addons as needed.

This approach is a lot simpler and easier to follow than the one
originally done in #419. It also requires less code changes.

Closes #157

---------

Co-authored-by: Anthony <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants