Skip to content

Conversation

shareefalis
Copy link
Contributor

@shareefalis shareefalis commented Apr 13, 2020

@shareefalis shareefalis requested a review from sandersn as a code owner April 13, 2020 22:10
@msftclas
Copy link

msftclas commented Apr 14, 2020

CLA assistant check
All CLA requirements met.

@shareefalis
Copy link
Contributor Author

shareefalis commented Apr 14, 2020

For a more robust implementation. SpeechRecognitionErrorEvent type has to be implemented but the spec is a draft and subject to changes. So ErrorEvent is a good enough implementation.
Reference: https://developer.mozilla.org/en-US/docs/Web/API/SpeechRecognitionErrorEvent

@saschanaz
Copy link
Contributor

ErrorEvent has nothing to do with SpeechRecognitionErrorEvent. This is done because no browser implements the interface.

@sandersn
Copy link
Member

It's OK, I guess, to use a different subtype that's "close enough". I'm not a huge fan, but I appreciate that default is easy even if it's not correct.

In the event that SpeechRecognitionErrorEvent is implemented, the PR that switches ErrorEvent to SpeechRecognitionErrorEvent will need to find and refer to this PR to explain why it's the right change.

@sandersn sandersn merged commit 71a0666 into microsoft:master Jun 12, 2020
@saschanaz
Copy link
Contributor

saschanaz commented Jun 12, 2020

@sandersn Despite the name, SpeechRecognitionErrorEvent has no filename, lineno, colno etc. from ErrorEvent and thus can confuse users more.

@sandersn
Copy link
Member

@orta and I had a hard time deciding, but it seems like the choice is basically between a type with too many properties, ErrorEvent and a type with too few, Event. ErrorEvent is actually an unrelated subtype of Event, but that's mainly confusing to maintainers.

I went with the too-large type because it at least provides error and message even if filename/lineno/colno are also mistakenly present. If you feel strongly about this I will revert it.

@saschanaz
Copy link
Contributor

That rationale sounds reasonable. Since the names of the extra properties strongly suggests they are irrelevant here, I don't oppose here. Thanks for the details behind the decision 👍

@shareefalis shareefalis deleted the patch-1 branch June 12, 2020 18:39
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.

4 participants