search

w3c / performance-timeline

Performance Timeline
https://w3c.github.io/performance-timeline/
Other
111 stars 34 forks source link

Use of "SyntaxError" seems incorrect, should be TypeError #163

Closed noamr closed 4 years ago

noamr commented 4 years ago

In 3.3.1, it is specified that when entryTypes/type is omitted, a SyntaxError should be thrown. However, SyntaxError are reserved for parsers (like JS syntax, CSS syntax) - not to semantic exceptions such as this one, which should probably be a TypeError.

npm1 commented 4 years ago

This was added in https://github.com/w3c/performance-timeline/pull/112. @yoavweiss do you recall if we considered TypeError here or not? I think I'd be ok changing it to TypeError.

yoavweiss commented 4 years ago

Past me thought SyntaxError seems like the best fit.

I believe we were aiming for a "SyntaxError" DOMException and not a simple exception, so IIUC it doesn't necessarily refer to parsers. I agree it's not ideal, but so does TypeError. A BadInput exception would've been ideal.

noamr commented 4 years ago

OK; Maybe then separate PerformanceObserverInit to PerformanceObsrverInitSingle with type+bufferred and PerformanceObserverInitMultiple with entryTypes? That way the IDL-generated code can emit this syntax error automatically... If there is some concensus on this I can suggest a PR

yoavweiss commented 4 years ago

That seems like a not-web-exposed change, and could potentially simplify the processing, and enable us to deprecate the "multiple" type in the future.

@npm1 - thoughts?

npm1 commented 4 years ago

Can you clarify precisely what the proposed IDL change is here? I'm not sure I follow.

noamr commented 4 years ago

Can you clarify precisely what the proposed IDL change is here? I'm not sure I follow.

Sure! something like:

` dictionary PerformanceObserverInitMultple { sequence entryTypes; };

dictionary PerformanceObserverInitSingle { DOMString type; boolean buffered; };

... void observe (optional (PerformanceObserverInitSingle or PerformanceObserverInitMultiple) options = {}); `

npm1 commented 4 years ago

Oh I see. Assuming we're sure that it's valid IDL, I'm ok with that.

npm1 commented 4 years ago

I started writing a PR for this but then I realized that the suggestion is not possible. Per https://heycam.github.io/webidl/#ref-for-dfn-distinguishable%E2%91%A2 you can only use distinguishable types in an 'or', and dictionaries are not distinguishable from each other.

So I'll just send a PR to change the error to TypeError.