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

Add proposal for a no-callbacks data guideline #14302

Merged
merged 3 commits into from May 4, 2022

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented Dec 29, 2021

Summary

Add a guideline to codify that we don't include non-exposed callback interfaces as standalone interfaces in BCD.

Test results and supporting details

This copies heavily from the mixin guideline.

There's one iffy thing here: the example provided is one we don't actually follow in practice. Maybe there's a better example?

Related issues

Fixes #3068.

@github-actions github-actions bot added the docs ✍️ Issues or pull requests regarding the documentation of this project. label Dec 29, 2021
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

LGTM, very descriptive!

@foolip
Copy link
Collaborator

foolip commented Dec 29, 2021

Web IDL callback interfaces can be observable to web developers, most easily if the interface has constants on it, as in the case of NodeFilter. Should https://developer.mozilla.org/en-US/docs/Web/API/NodeFilter really be removed?

Callback functions don't have names so I don't think a guideline to prevent them from being included in BCD is necessary. Or are there any?

@ddbeck
Copy link
Collaborator Author

ddbeck commented Dec 31, 2021

Web IDL callback interfaces can be observable to web developers, most easily if the interface has constants on it, as in the case of NodeFilter. Should https://developer.mozilla.org/en-US/docs/Web/API/NodeFilter really be removed?

No, that's not intentional. I didn't know that there were any exposed callback interfaces (and, so far, I've only seen NodeFilter). Though it's still an open question whether we need a guideline for this, I've revised the text to exclude [Exposed] interfaces. I suppose another approach might be a more general guideline: no unexposed (non-exposed? not sure on terminology here) names in api.

Callback functions don't have names so I don't think a guideline to prevent them from being included in BCD is necessary. Or are there any?

Historically, there were some, though I don't know if every one has been purged. Resolving everything in #6810 might settle that question. In any case, we don't add them now but haven't really said so explicitly.

@queengooborg
Copy link
Collaborator

Doing a quick search through the @webref/idl package, it appears that there is a total of three callback interfaces: EventListener, NodeFilter, and XPathNSResolver. Only the first two are in BCD, and only one of them, NodeFilter, is exposed. I feel like it's also worth pointing out that the mdn-bcd-collector project doesn't generate tests for these callback interfaces, as the WebIDL parser (webidl2 by W3C) doesn't consider them to be interfaces.

Personally, I'm voting for just removing all callback data. I feel that their functionality is sufficiently represented by the features they're used in, and otherwise well documented on MDN web docs.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

The guideline as is works for me.

I think I'm with @queengooborg on this one. So, I also think we could go further and just don't record callbacks compat data of any kind.

@ddbeck
Copy link
Collaborator Author

ddbeck commented Jan 26, 2022

@foolip did you have any more thoughts on this? Not sure if I overcame your objections or not.

@foolip
Copy link
Collaborator

foolip commented Feb 1, 2022

@ddbeck the direction here seems like the right one, but I'm actually still not sure what it will mean in practice in multiple cases.

Should https://developer.mozilla.org/en-US/docs/Web/API/NodeFilter#browser_compatibility remain? The NodeFilter callback interface object is exposed because it has constants on it, but we don't record constants in BCD. And acceptNode isn't a member in the usual sense, it's more like a dictionary member, something that the browser will look for on objects passed in where a NodeFilter argument is expected. I don't think there's anything worth keeping in the NodeFilter compat data, as it turns out.

For EventListener, is there really something that should be recorded as a subfeature of api.EventTarget.addEventListener as the guideline suggests? What is that something? It would have the same versions as the parent feature, and go back to browser antiquity, so it doesn't seem worth recording to me.

@ddbeck
Copy link
Collaborator Author

ddbeck commented Apr 12, 2022

For NodeFilter, I think we can make a judgment that it's sufficiently dict-like to remove it, even though it's not technically one. Ostensibly, this new guideline would address it, but if it quacks like a duck…

For EventListener, I think this guideline speaks directly to it. We have data for the EventListener "interface". But it's not exposed. If there's anything interesting to be reported about the EventListener callback, then it should be as a subfeature of addEventListener, not as api.EventListener alone.

That said, I'd be open to fixing those two cases and moving on without a guideline entirely, if those are the only two cases that this guideline would tend to.

@queengooborg
Copy link
Collaborator

I opened two PRs to remove NodeFilter and EventListener in #15783 and #15785, taking care of the two remaining callbacks in our data! I do still think we should add a guideline for callbacks, as there are many other callbacks we have removed in the past.

@queengooborg
Copy link
Collaborator

Question for everyone: what do we need to do to move this PR forward? I'm not quite certain what we'd need to do next, if anything.

@queengooborg
Copy link
Collaborator

This was approved during the bi-weekly BCD meeting, so this is LGTM!

@queengooborg queengooborg merged commit a272624 into mdn:main May 4, 2022
@ddbeck ddbeck deleted the no-callbacks branch May 5, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs ✍️ Issues or pull requests regarding the documentation of this project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need guidance for callback functions.
4 participants