Use an AddonsApplication to manage load/reload of all addons

[humitos]

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

[humitos]

I was able to make everything work here 🎉 and I'm very happy with the solution I arrived at. It's a lot simpler than the original proposal. This PR is ready for review.

[agjohnson]

Hrm, I'm a bit confused on this pattern, there seems to be a few patterns mixing here.

Why use the context API at all here? The context API is specifically for reactive element properties, so if this context isn't read from each addon element (this.config.getValue), it's the same thing as just passing in a native object to the addon and skipping the context API entirely.

What you have here doesn't seem like it will give a reactive property to each addon -- if we update config, these updates won't flow to the addon elements. How important is this?

With this approach, we also don't need a readthedocs-application element. We're not using the context API as reactive element properties, so an Element isn't needed to execute logic. All of this logic can happen at index.js and without any elements being registered -- instead of injecting and element and forcing render() to execute this logic.

It's a little difficult to follow but it feels like what you have here could be simplified down to executing this logic in index.js, outside any element, and pushing the config to each addon. But this is essentially where we are currently. Is there something this PR does that we're not doing currently?

[humitos]

Is there something this PR does that we're not doing currently?

Yes.

1. This PR listen to EVENT_READTHEDOCS_URL_CHANGED --which is triggered by ourselves when we detect the URL has changed using the history object (eg. SPA, like Docusaurus or Material forMkDocs with a specific config)
2. The handler of that event, makes a new requests to the Addons API and fetch the updated data based on the new URL.
3. Once the data is retrieved, the event EVENT_READTHEDOCS_ADDONS_DATA_READY is fired.
4. The handler of this event updates the ContextProvider with the data fetched by the API.
5. The AddonsApplication is subscribed as a ContextConsumer and will re-initialize all the addons with the new data (config object).
6. At this point, all the addons were updated with the new data (eg. flyout URLs now point to the new URL)

Currently, without the code of this PR, the Addons API is fetch only once when the page is loaded. If the user clicks on another page that doesn't perform a full reload (eg. as Docusaurus does) there is no new request to Addons API and addons are not re-initialized. That means that all the URLs from the flyout are out of date and point to the wrong page.

You can try the current behavior following these steps:

1. Go to https://test-builds.readthedocs.io/en/docusaurus/docs/intro
2. Take a look at the URLs in the flyout (they should contain /docs/intro)
3. Click on "Next" link at the bottom
4. Take a look at the URLs in the flyout again (they will contain /docs/intro -- which is wrong, since the page has changed)

[humitos]

The question is: can we use ContextProvider / ContextConsumer outside a LitElement? I've created a AddonsApplication to be able to subscribe as a ContextConsumer, but maybe that's not needed. I understand that's what confuses you about this pattern.

We don't require a reactive property on each addon -- we only need to subscribe as a ContextConsumer and re-initialize the addons with the fresh data. If we can make the AddonsApplication to not be a LitElement and still use the ContextConsumer for this, that would be great. I will give it a try!

[humitos]

If we can make the AddonsApplication to not be a LitElement and still use the ContextConsumer for this, that would be great. I will give it a try!

It seems that's not possible. I gave it a quick try and I got the error:

Uncaught TypeError: this.host.addController is not a function

[humitos]

Boom! 💯

Lit Context are not required at all for what I want to do, actually. Thanks for your comment, it helped me to understand the workflow a lot better. I'm using just events to communicate internally and pass the config data we need 👍🏼

I re-wrote the code to remove everything related to Lit Context. The structure was correct (using a manager class, like AddonsApplication) and was pretty easy to migrate it to regular events.

Let me know your thoughts! I think it's very clear now.

[humitos]

I want to deploy this implementation since we are promoting more and more new documentation tools and some of them are SPA: docusaurus, markdoc, vitepress, etc...

[agjohnson]

We don't require a reactive property on each addon -- we only need to subscribe as a ContextConsumer and re-initialize the addons with the fresh data.

We do still need a reactive config property on each addon Element, we want our Element classes to render updates on changes to config.

I found the pattern here a bit hard to digest, it seems at odds with how Element properties work normally. On our elements, config should be a reactive property if we're expecting render() to use the values to conditionally alter rendering.

I guess this feels odd because this has created a secondary layer for a reactive config property and it's relying on all of the added steps of re-initialization and orphaning of the existing *Addon instances to accomplish this reactive-ish property. That is, config is a reactive property that will re-render the Element natively already, but this is also duplicating that functionality through reinitializing the *Addon classes.

This does feel like it could open us up to odd side effects from *Addon class initialization when we really just want to pass config to the elements that were found or created the first time *Addon was initialized.

While I think the context API is much clearer in what it's accomplishing, this should at very least avoid recreating/finding elements unnecessarily. Once we initialize *Adddon classes, these classes should have reference of the elements they are connected to. But again, the context API does this for us already.

The question is: can we use ContextProvider / ContextConsumer outside a LitElement?

I've shown an example of using ContextProvider already at:

addons/src/context.js

Lines 11 to 21 in 1c3032b

	/**
	* Because `config` provider is not attached to a ReactiveElement, and is
	* instead connected to `document.html`, we have to call `hostConnected()`
	* manually. See:
	*
	* https://github.com/lit/lit/blob/935697d47e62ed75e3157423400163a8371c62fc/packages/context/src/lib/controllers/context-provider.ts#L55-L58
	**/
	const config = new ContextProvider(document.documentElement, {
	context: configContext,
	});
	config.hostConnected();

I did get a similar error using ContextProvider on document.html initially, which is why that note is on that code. It was a similar error to what you got on the consumer:

Uncaught TypeError: this.host.addController is not a function

Looking at the code though, this error is unavoidable as the error happens in the constructor.

However, it's still possible to use ContextRequestEvent directly, which is what ContextConsumer configures under the hood. This would probably live as an event callback on the *Addon classes instead of the *Element classes as a ContextConsumer.

[humitos]

Thanks for your feedback here.

After trying different pattern for this, I personally found the one from this PR the easiest to follow, understand and implement. I wrote about the problems/cons I've found with ContextRoot in #491 (comment) and those were key to decide continue in a different direction. It may not be perfect, but it seems fine for now.

I guess this feels odd because this has created a secondary layer for a reactive config property and it's relying on all of the added steps of re-initialization and orphaning of the existing *Addon instances to accomplish this reactive-ish property. That is, config is a reactive property that will re-render the Element natively already, but this is also duplicating that functionality through reinitializing the *Addon classes.

I understand this. However, one of the big downsides of making config a re-active property and use ContextRoot for this, is that we have to move all the validation logic inside render() -- which I found pretty complex and and kind of an anti-pattern, as I mentioned in the comment linked below.

In our context, I see the change in the config more as a re-initialization than a re-renderization, because we have to run all the validation checks on the data before performing the render --the same validation checks we do when we initialize the addon. That makes sense to me.

Also note than moving the validation checks inside render(), these checks will be executed multiple times without being required. Example: when the user clicks on the flyout to open/close it. In that case, we will call render() and re-ran all the validation checks even if nothing in the config has changed.

This does feel like it could open us up to odd side effects from *Addon class initialization when we really just want to pass config to the elements that were found or created the first time *Addon was initialized.

I wasn't able to trigger the side effect you mention here (the logic we have in the Addon constructor() already considers checking for existing elements before creating a new one). I'm fine adding another check if needed, tho.

---

Summarizing, from all the patterns I've tried/implemented here, the one I feel more comfortable with and the one that I was able to make it work is this PR. I know that it's not the best one and it can be improved, but I wasn't even able to finish the work using ContextRoot and while working on that one I found it pretty complex to do it correctly. Besides, the data validation being inside the render() is a no-go to me.

That said, I'm happy to continue exploring a different pattern if we want to; but I prefer to move forward with the current one as it's working and finished. I wasn't able to arrive at at working pattern with all the other things I've tried. This PR won't affect how users interacts with out addons, so I propose to merge this PR and open a bigger conversation/discussing where we can continue exploring different/better alternatives without blocking the feature and giving a solution to those users using SPA documentation tools. @agjohnson what do you think?

[humitos]

this should at very least avoid recreating/finding elements unnecessarily. Once we initialize *Adddon classes, these classes should have reference of the elements they are connected to

I went ahead and implemented this. Now, we call new *Addon() only once when the page loads for the first time and we save all the instances created. After that, every time the "URL changed" event is fired, we iterate over the saved instances and call .loadConfig(config) on them 👍🏼

[agjohnson]

I wrote about the problems/cons I've found with ContextRoot in #491 (comment)

Yes, already saw that previously. I described above using the context API from the *Addon classes instead of from our elements which I think avoids most of the concerns you listed.

However, one of the big downsides of making config a re-active property and use ContextRoot for this, is that we have to move all the validation logic inside render()

I wasn't describing moving validation logic into render(), I understand the concerns of validation/etc on render. I described above using the context API ContextRequestEvent from *Addon instead above.

This would still use a reactive property config in our elements. We are already using a reactive config property in our elements, this doesn't change. The change would be using the context API from *Addon and updating the config property on connected elements.

I wasn't able to trigger the side effect you mention here

Side effects will happen without us noticing or knowing what to watch. Logic beyond validation and updating the element config -- subscribing to events, emitting events, whatever -- will happen unless we're careful to avoid these.

That is why if we only need validation and to update config, I feel it's best to limit our logic to just that.

every time the "URL changed" event is fired, we iterate over the saved instances and call .loadConfig(config) on them

This is safer for now. If we're able to use the context API, the code will look similar to this -- we won't reinitialize each addon again.

[agjohnson]

Could you describe this in more detail here? What race condition?

I feel this should be a solvable problem, but likely comes from using promises to initialize addons asynchronously? If we truly want promises handling the addon initialization, we can structure the promises to respect initialization order.

[humitos]

I didn't write that note. I just copied and pasted from another addon while doing the refactor. I think you wrote it when you started with the flyout work 😄

In any case, I tried using render() and it doesn't work 🤷🏼

[humitos]

This is safer for now. If we're able to use the context API, the code will look similar to this -- we won't reinitialize each addon again.

Cool! I will keep this in mind and keep experimenting with Context API in future iteration. I think it's still useful to give it a try if we can remove all the complexity I found myself limiting moving forward with that. I suppose we can change the "base event handling at ApplicationApp for Context API" and everything should keep working in a similar way --but I don't want to open that melon now 😄