Skip to content

Make MIDIMessageEventInit data and MIDIConnectionEventInit port explicitly nullable #257

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

Conversation

mjwilson-google
Copy link
Contributor

#252 made MIDIMessageEvent data and MIDIConnectionEvent port nullable.

Based on this comment: #233 (comment) I didn't make the init dictionary members nullable.

However, Chromium currently has these dictionary members nullable:
https://crsrc.org/c/third_party/blink/renderer/modules/webmidi/midi_connection_event_init.idl;l=11
https://crsrc.org/c/third_party/blink/renderer/modules/webmidi/midi_message_event_init.idl;l=11

And Firefox has port nullable (but not data):
https://searchfox.org/mozilla-central/source/dom/webidl/MIDIConnectionEvent.webidl#23
https://searchfox.org/mozilla-central/source/dom/webidl/MIDIMessageEvent.webidl#24

Since the intent was to codify the existing Chromium behavior, I think this means that we should make them both nullable.

This actually does make a difference, since testing in Chromium we get the current behavior:

new MIDIMessageEvent("something", {data: null}).data
null

new MIDIConnectionEvent("something", {port: null}).port
null

But after aligning the IDL with the current spec we get the following:

new MIDIMessageEvent("something", {data: null}).data
VM261:1 Uncaught TypeError: Failed to construct 'MIDIMessageEvent': Failed to read the 'data' property from 'MIDIMessageEventInit': Failed to convert value to 'Uint8Array'.
at :1:1

new MIDIConnectionEvent("something", {port: null}).port
VM285:1 Uncaught TypeError: Failed to construct 'MIDIConnectionEvent': Failed to read the 'port' property from 'MIDIConnectionEventInit': Failed to convert value to 'MIDIPort'.
at :1:1

@padenot especially, what do you think?

@mjwilson-google mjwilson-google self-assigned this Oct 19, 2023
@mjwilson-google
Copy link
Contributor Author

This is related to #168 and #233.

@mjwilson-google mjwilson-google added this to the CR milestone Oct 19, 2023
@mjwilson-google
Copy link
Contributor Author

After further discussion with the Chromium API owners, this change isn't appropriate. Even though it will change Chromium behavior in this specific case, keeping dictionary members not required and not adding the ? is better for web platform conventions so we shouldn't merge this change. I will close this.

@mjwilson-google mjwilson-google deleted the nullable-dictionaries branch July 9, 2024 23:23
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.

1 participant