Improvements to Web IDL exception message strings

Hello all,

We just made some changes to how exception strings from ErrorResult get 
formatted [1].  The changes apply to both the Throw* DOMException 
methods and ThrowTypeError/ThrowRangeError.

In the new setup, exception messages have "%s: " prepended to them, 
where %s is replaced with one of:

* "InterfaceName.methodName", when the exception is from a method cal.
* "InterfaceName.attrName getter", when the exception is from a getter
* "InterfaceName.attrName setter", when the exception is from a setter
* "InterfaceName constructor", when the exception is from a constructor.

The idea is to give web developers a bit more context about what callee 
failed.

I have tried to go through existing message strings and remove 
duplication of that information where it existed.

The changes only affect exceptions thrown via the ErrorResult that 
bindings pass to implementation methods.  In particular, Promise 
rejections via Promise methods (as opposed to throwing on the passed-in 
ErrorResult) do not benefit from these changes; more work will need to 
happen there.

The changes also do not affect exceptions thrown via 
ErrorResult::Throw(nsresult).  As a reminder, this is deprecated and 
people should switch to the methods that provide useful error messages.

Please let me know if there are concerns around this change, suggestions 
for further improvements, etc!

-Boris

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613013
0
Boris
2/7/2020 1:55:39 PM
mozilla.dev.platform 6600 articles. 0 followers. Post Follow

5 Replies
19 Views

Similar Articles

[PageSpeed] 0

On Friday, February 7, 2020 at 8:55:45 AM UTC-5, Boris Zbarsky wrote:
> Hello all,
> 
> We just made some changes to how exception strings from ErrorResult get 
> formatted [1].  The changes apply to both the Throw* DOMException 
> methods and ThrowTypeError/ThrowRangeError.
> 
> In the new setup, exception messages have "%s: " prepended to them, 
> where %s is replaced with one of:
> 
> * "InterfaceName.methodName", when the exception is from a method cal.
> * "InterfaceName.attrName getter", when the exception is from a getter
> * "InterfaceName.attrName setter", when the exception is from a setter
> * "InterfaceName constructor", when the exception is from a constructor.

Would you consider using something that avoids the confusion with static methods/properties? E.g.

- InterfaceName's methodName()
- InterfaceName's attrName getter
- InterfaceName's attrName setter

As-is it seems like it would be confusing to talk about, for example, Node.appendChild, which is === undefined.

I'm also curious what the output would be for actually-static methods, e.g. CSS.escape(), DOMPointReadOnly.fromPoint(), etc.

> 
> The idea is to give web developers a bit more context about what callee 
> failed.
> 
> I have tried to go through existing message strings and remove 
> duplication of that information where it existed.
> 
> The changes only affect exceptions thrown via the ErrorResult that 
> bindings pass to implementation methods.  In particular, Promise 
> rejections via Promise methods (as opposed to throwing on the passed-in 
> ErrorResult) do not benefit from these changes; more work will need to 
> happen there.
> 
> The changes also do not affect exceptions thrown via 
> ErrorResult::Throw(nsresult).  As a reminder, this is deprecated and 
> people should switch to the methods that provide useful error messages.
> 
> Please let me know if there are concerns around this change, suggestions 
> for further improvements, etc!
> 
> -Boris
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613013

0
Domenic
2/8/2020 7:31:04 PM
On 2/8/20 2:31 PM, Domenic Denicola wrote:
> Would you consider using something that avoids the confusion with static methods/properties? E.g.
> 
> - InterfaceName's methodName()
> - InterfaceName's attrName getter
> - InterfaceName's attrName setter

We could think about doing that, yes.  If we do, we should change all 
the existing messages that talk about InterfaceName.methodName (e.g. the 
ones that get thrown for invalid arguments, etc).

The main goal here is to give web developers an idea of which method 
they were calling quickly, without taking up space from the actual error 
message which tells them what it was they did wrong in calling it.  So 
there's a tension between brevity and getting the details right...  Your 
suggestions are pretty decent on the brevity front, though, so that 
seems workable.

I wonder what web developers think of the options here...

> I'm also curious what the output would be for actually-static methods, e.g. CSS.escape(), DOMPointReadOnly.fromPoint(), etc.

It'd be "CSS.escape: ", etc.

You can test this now in the console using:

   CSS.escape() /* no args */
   document.getElementById() /* no args */

My changes did not affect the error messages from those, note; just used 
that same string in more places.

-Boris
0
Boris
2/9/2020 1:35:22 AM
On Mon, Feb 10, 2020 at 3:23 PM Boris Zbarsky <bzbarsky@mit.edu> wrote:
> You can test this now in the console using:
>
>    CSS.escape() /* no args */
>    document.getElementById() /* no args */
>
> My changes did not affect the error messages from those, note; just used
> that same string in more places.

Would using Document.prototype.getElementById be too long? I've
sometimes seen developers advocate using # to mean .prototype (e.g.,
Document#getElementById via
https://mathiasbynens.be/notes/javascript-prototype-notation), but I
don't know how widespread that is. It does seem a little funky to use
Document.getElementById.
0
Anne
2/10/2020 2:57:49 PM
On 2/10/20 9:57 AM, Anne van Kesteren wrote:
> Would using Document.prototype.getElementById be too long?

That is a good question.

I'm going to do a bit of refactoring so we only need to change one place 
to try these ideas out and would love some feedback!

In the meantime, someone who wants to try this locally should apply the 
patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1614471 and 
experiment a bit.

-Boris
0
Boris
2/10/2020 9:12:15 PM
Hi,

helpful exception messages are very important, and ensuring that
sufficient context is present is a good contribution to that. Thanks
for pushing this forward!

When starting work on replacing uses of the deprecated
ErrorResult::Throw(nsresult), I noticed that the existing error
messages show a syntactical inconsistency. Some end with a period (.),
some do not. To ensure a consistent presentation, we should define the
preferred style for this, and apply it consistently.

I think the options with their rationale are:

1. individual messages passed to Throw* should not end in a period,
but should form a complete sentence otherwise and a period suffix
might be added depending on how/where it is exposed

2. all individual messages passed to Throw* should end in a period,
and they form a complete sentence; no additional period is ever added
regardless of how they are exposed

3. individual messages passed to Throw* may or may not end in a
period, depending on whether they form a complete sentence; no
additional period is ever added regardless of how they are exposed

I think either of the first two options should be chosen, with a
slight preference for the first option.

Are there other opinions on this? Maybe there are more options I
didn't consider.

Simon


On Mon, Feb 10, 2020 at 3:23 PM Boris Zbarsky <bzbarsky@mit.edu> wrote:
>
> Hello all,
>
> We just made some changes to how exception strings from ErrorResult get
> formatted [1].  The changes apply to both the Throw* DOMException
> methods and ThrowTypeError/ThrowRangeError.
>
> In the new setup, exception messages have "%s: " prepended to them,
> where %s is replaced with one of:
>
> * "InterfaceName.methodName", when the exception is from a method cal.
> * "InterfaceName.attrName getter", when the exception is from a getter
> * "InterfaceName.attrName setter", when the exception is from a setter
> * "InterfaceName constructor", when the exception is from a constructor.
>
> The idea is to give web developers a bit more context about what callee
> failed.
>
> I have tried to go through existing message strings and remove
> duplication of that information where it existed.
>
> The changes only affect exceptions thrown via the ErrorResult that
> bindings pass to implementation methods.  In particular, Promise
> rejections via Promise methods (as opposed to throwing on the passed-in
> ErrorResult) do not benefit from these changes; more work will need to
> happen there.
>
> The changes also do not affect exceptions thrown via
> ErrorResult::Throw(nsresult).  As a reminder, this is deprecated and
> people should switch to the methods that provide useful error messages.
>
> Please let me know if there are concerns around this change, suggestions
> for further improvements, etc!
>
> -Boris
>
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613013
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
0
Simon
2/11/2020 5:30:21 PM
Reply: