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.

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

CustomEvent: communicate subcribed to event without meta tag defined #279

Closed
humitos opened this issue Mar 19, 2024 · 6 comments
Closed
Assignees
Labels
Improvement Minor improvement to code

Comments

@humitos
Copy link
Member

humitos commented Mar 19, 2024

It would be good if we can communicate to the user they are subscribed to the readthedocs-addons-data-ready but they haven't defined the meta[name=readthedocs-addons-api-version] tag which is mandatory to make usage of the Read the Docs data event.

Reference: https://github.com/readthedocs/addons/pull/64/files#r1331997049

@humitos
Copy link
Member Author

humitos commented Mar 25, 2024

I had an idea about this during the weekend and I think it could be good. I'm writing it down here to receive some feedback and consider implementing it.

When triggering readthedocs-addons-data-ready instead of passing the raw JSON object, we could pass an instance of a class we control, e.g (AddonsData). This class requires calling .initialize() to be able to access the JSON object with the addons data. After that, the user can access AddonsData.config which contains the full JSON data

This allows us to perform anything we need inside the .initialize() method:

  • tell users that they need to define the meta HTML tag
  • communicate anything new we need in the future
  • raise exceptions to block the usage of the JSON data if any validation is no passed
  • eventually hit an API to log who is using this custom event (?)
  • ... and any other requirement we may have in the future

In other words, I'm suggesting to pass an instance of a class we control and enforce users to call a method before making usage of the data. I think this solves the problem I already raised but also gives us the flexibility in the future to communicate with users. @agjohnson what do you think?

@humitos
Copy link
Member Author

humitos commented Mar 25, 2024

The usage from a user's perspective would like this:

document.addEventListener("readthedocs-addons-data-ready", function(event) {
    const addons = event.detail;

    // Accessing `addons.data` at this point without initializing it will fail.
    // It will raise an exception at this point communicating what happened.

    // So, we require users calling `addons.initialize()` here.
    addons.initialize();
    
    // Then, the user can access to the data and do whatever they need.
    const config = addons.data;

    ....
});

@agjohnson
Copy link
Contributor

I think I get what you're aiming for, this seems like a good way to solve the issue above. I would agree that providing a richer data structure in some way would help us now, and probably in the future too.

We could also pass the user a callback function if we don't see the need for additional class methods/etc.

That is:

document.addEventListener("readthedocs-addons-data-ready", function(event) {
    const fn = event.detail;
    const config = fn();
    ...
});

This is maybe a more an expected pattern in JS but the two seem functionally the same.

@humitos
Copy link
Member Author

humitos commented Mar 25, 2024

We could also pass the user a callback function if we don't see the need for additional class methods/etc.

Yeah, I was suggesting a class since seems more flexible for future scenarios --but I've never done this, so I'm not sure if it's a good idea or not.

This is maybe a more an expected pattern in JS but the two seem functionally the same.

Does this pattern have a name I can Google for?1 Do you have a link at hand where I can read more about this?

Footnotes

  1. I thought I was inventing something revolutionary here, but it seems it already exists 😝

@agjohnson
Copy link
Contributor

Does this pattern have a name I can Google for?

Good question. Maybe "callback" is the closest? This feels maybe similar to how a JSONP callback might work, just with an event added in instead of a named function.

@humitos
Copy link
Member Author

humitos commented Apr 9, 2024

I implemented an approach that I like in #287

@humitos humitos moved this from Planned to Needs review in 📍Roadmap Apr 10, 2024
humitos added a commit that referenced this issue Apr 11, 2024
I found a pattern that I like 👍🏼 . I think it's a clear way to
communicate users what's going on, but also leaves the door open to
expand checks/validations in the future. I also imagine we could even
want to know "What projects are subscribing to the custom event?" in
case we need to contact them due to a breaking change or similar.

## Examples

#### Calling `event.detail.initialize()` without having defined the
`<meta ... />` HTML tag:


![Screenshot_2024-04-09_13-57-20](https://github.com/readthedocs/addons/assets/244656/b1757a08-aa44-4c3b-a5c2-8dbe73a477b4)

#### Calling `event.detail.data()` without calling `.initialize()`
first:


![Screenshot_2024-04-09_13-56-01](https://github.com/readthedocs/addons/assets/244656/476a934b-7668-4a58-8dbc-8e4254f6049c)


Closes #279
@humitos humitos closed this as completed Apr 11, 2024
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
Archived in project
Development

No branches or pull requests

2 participants