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: pass a ReadTheDocsData object as event.detail #287

Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Apr 9, 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

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

Screenshot_2024-04-09_13-56-01

Closes #279

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted how this pattern could be improved for the user. The class structure isn't a problem, but could be easier as a function too, the class is maybe not adding much.

src/events.js Outdated Show resolved Hide resolved
src/events.js Outdated Show resolved Hide resolved
@@ -126,7 +128,7 @@ export function getReadTheDocsConfig(sendUrlParam) {
dispatchEvent(
EVENT_READTHEDOCS_ADDONS_DATA_READY,
document,
dataUser,
new ReadTheDocsEventData(dataUser),
Copy link
Contributor

@agjohnson agjohnson Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in terms of resolving the promise before calling the event, dispatchEvent would trigger inside a promise then() that resolves dataUser

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand your comment here. Can you expand on this? 🙏🏼

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this comment a couple of times and I also tested this locally to find our how to resolve this Promise. I made these changes in a92994a. Let me know if that sounds good to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what I mean is that response.json()/dataUser is a promise, and we should give the user the actual value that is provided by resolving/fulfilling response.json(). You seem to have it close I think, but I feel like your example might be prematurely resolving the parent Promise perhaps. Let me digest this one a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, see #286 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just commented on the commit directly a92994a

But to keep conversation here, you may have resolved the wrong promises (both the API promises and the user JSON data promises).

@humitos
Copy link
Member Author

humitos commented Apr 10, 2024

The class structure isn't a problem, but could be easier as a function too, the class is maybe not adding much.

I also think the class is not adding too much right. However, do you think we will have the same flexibility with a function in the future in case we need to perform more validations/checks? I'm asking because my limited js knowledge 😅

@humitos humitos marked this pull request as ready for review April 10, 2024 09:58
@humitos humitos requested a review from a team as a code owner April 10, 2024 09:58
@humitos humitos requested a review from agjohnson April 10, 2024 09:58
@agjohnson
Copy link
Contributor

However, do you think we will have the same flexibility with a function in the future in case we need to perform more validations/checks? I'm asking because my limited js knowledge

We would have plenty of flexibility to include logic that we want to hide from the maintainer user. But, if we wanted to expose and public functions/methods, the class structure would make this cleaner. I think it's fine to stick with the class based approach if we aren't sure or are leaning towards exposing public methods -- ie, maybe a event.detail.getVersions() helper method or something similar.

@humitos
Copy link
Member Author

humitos commented Apr 11, 2024

It seems we both agree on passing a class instead of the raw JSON data. So, I'm merging this PR into the one its based, and we can continue with the minor details there.

@humitos humitos merged commit 4c7833f into humitos/custom-event-promise Apr 11, 2024
2 of 4 checks passed
@humitos humitos deleted the humitos/custom-event-promise-object branch April 11, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants