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

Updated Notification APIs #3667

Merged
merged 8 commits into from
Oct 27, 2023
Merged

Updated Notification APIs #3667

merged 8 commits into from
Oct 27, 2023

Conversation

maccesch
Copy link
Contributor

No description provided.

@maccesch
Copy link
Contributor Author

I don't exactly understand why this on check didn't go through. I followed the contribution guidelines. What can I do to fix this?

@daxpedda
Copy link
Collaborator

This is just missing the Cargo.toml adjustments, looking at the CI results you can easily see what needs to be done:

--- a/crates/web-sys/Cargo.toml
+++ b/crates/web-sys/Cargo.toml
@@ -880,6 +880,7 @@ NodeFilter = []
 NodeIterator = []
 NodeList = []
 Notification = ["EventTarget"]
+NotificationAction = []
 NotificationBehavior = []
 NotificationDirection = []
 NotificationEvent = ["Event", "ExtendableEvent"]

I don't exactly understand why this on check didn't go through. I followed the contribution guidelines. What can I do to fix this?

You probably forgot to point the tool towards the Cargo.toml file. The issue is that the book isn't updated, but you find the updated version here: https://rustwasm.github.io/wasm-bindgen/contributing/web-sys/supporting-more-web-apis.html.
Note the last argument ./Cargo.toml.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Oct 24, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Could you also add the corresponding fields to Notification?
See https://notifications.spec.whatwg.org/#api.

@maccesch
Copy link
Contributor Author

I replaced the whole idl file now with the latest spec from that link. I hope that's ok? It makes some breaking changes but the previous version of the notification API seems to have been very old.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Unfortunately we can't just correct the API like this because that would require a breaking change.

I think removing GetNotificationOptions, NotificationBehavior and Notification.get() is fine though because they don't actually exist anymore. @Liamolucko and @hamza1311 would you be fine with that as well?

Could you also add a note in the changelog? Especially mention the removal of these APIs separately.

crates/web-sys/webidls/enabled/Notification.webidl Outdated Show resolved Hide resolved
crates/web-sys/webidls/enabled/Notification.webidl Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: daxpedda <daxpedda@gmail.com>
@daxpedda daxpedda added needs review Needs a review by a maintainer. and removed waiting for author Waiting for author to respond labels Oct 25, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Thank you, this is LGTM.
Just gonna wait for another maintainer to confirm the API removals.

@Liamolucko
Copy link
Collaborator

I think removing GetNotificationOptions, NotificationBehavior and Notification.get() is fine though because they don't actually exist anymore. @Liamolucko and @hamza1311 would you be fine with that as well?

Yeah, I'm fine with this.

@ranile
Copy link
Collaborator

ranile commented Oct 27, 2023

I'm fine with it. If it "breaks" any code, that code is already broken and now there is an error message for it at compile time.

@ranile ranile merged commit a03d23b into rustwasm:main Oct 27, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs a review by a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants