-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Data for legacy version of webextension.api.userScripts #25857
base: main
Are you sure you want to change the base?
Data for legacy version of webextension.api.userScripts #25857
Conversation
On reflection, don't believe this is the right approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mdn_url
is missing everywhere. Is that intended?
Other than that, this looks good at the surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you aligned on this approach with @Rob--W on Monday.
If the only overlap between the old and the new API is register()
, then an alternative solution would be to mark all methods from the old API as deprecated, and only rename the legacy register
BCD feature to register_legacy
(with a good description).
Anyways, if we decide to introduce webextensions.api.userScripts_legacy
, then shouldn't webextensions.api.userScripts
reflect support for the new API, even if that's currently false
for everything?
/cc @Elchi3 in case he has other ideas for this case.
@Rob--W |
Yes
I'm working on a PR to add the documentation for the new API now and will update the BCD shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a structure point of view, didn't verify the accurateness of the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a good place for adding a reference to the newer userScripts APIs for future reference?
I.e. a comment to the top level userScripts namespace, register method, RegisteredUserScripts, RegisteredUserScript.unregister methods.
"edge": "mirror", | ||
"firefox": { | ||
"version_added": "68", | ||
"notes": "For extension using Manifest V3, see [`userScripts.unregister`](https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/userScripts/unregister)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read this out of context, it could be read as the API being meant for extensions using MV3, and that there is more information at the given link. Is this just me?
A way to address that is to prepend a note like "Only available for use in extensions using Manifest V2."
"edge": "mirror", | ||
"firefox": { | ||
"version_added": "68", | ||
"notes": "For extension using Manifest V3, see [`userScripts.unregister`](https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/userScripts/unregister)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On rereading - I can only agree!
"notes": "For extension using Manifest V3, see [`userScripts.unregister`](https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/userScripts/unregister)." | |
"notes": "Only available for use in extensions using Manifest V2. For extension using Manifest V3, see [`userScripts.unregister`](https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/userScripts/unregister)." |
Summary
Initial PR for addressing the compatibility data needs of Bug 1943050 Enable userScripts API by default. This PR creates the compatibility data to complement the documentation changes in mdn/content#37975: