Opened 5 years ago

Closed 4 years ago

#18379 closed enhancement (wontfix)

[patch][cla] More graceful failure with string.substitute?

Reported by: dylan Owned by: dylan
Priority: undecided Milestone: 1.11
Component: String Version: 1.10.2
Keywords: Cc:
Blocked By: Blocking:

Description

I received the following request from a long time user:

I would like to propose one really simple, but cool improvement which I
use for a long time already.
In string.js there is the substitute function - important for all
translations in dojo. Why does it throw an exception if a key is not
found? A mysterical transform exception appears with no further
information - just if you forgot a translation or just have a typo in it.
Why not doing it like JSTL ?  Just using the key and adding ??? in front
and behind of it.
Put these lines in string.js from line 144
if (typeof value=='undefined') {
	value="???"+key+"???";
}
.. and you're really happy when you see in your application immediately
what hasn't been translated properly. You can fix it in seconds.
This helps a lot when coding and working with translations.
  1. Should we fail more gracefully?
  2. If yes, what approach should we take?

Change History (18)

comment:1 Changed 5 years ago by dylan

Per Bryan, a better option would be:

if (value === undefined) {
	console.warn(className + ' template couldn\'t find: ' + key);
	return match;
}

comment:2 Changed 5 years ago by dylan

Summary: More graceful failure with string.substitute?[patch][cla] More graceful failure with string.substitute?

comment:3 Changed 5 years ago by Adam Peller

FWIW, the choice to stop execution on unmatched keys was by design, as was the minimalist way it throws and lack of logging bloat. see https://bugs.dojotoolkit.org/ticket/3700#comment:3

comment:4 Changed 5 years ago by dylan

@peller, Yes, I'm not surprised it was intentional, though the logging would get removed with a build anyway. We could wrap this entire patch in something like an if(has("config-isDebug")) statement if we are concerned that it would truly impact performance, so that it's removed in production builds?

comment:5 Changed 5 years ago by Adam Peller

For the addition of logging, sure. We didn't have those features 7 years ago. But removing the exception would be a change to documented behavior, and lead to errors that are difficult to discover.

comment:6 Changed 5 years ago by dylan

What I mean is:

if (has('config-isDebug') && value === undefined) {
	console.warn(className + ' template couldn\'t find: ' + key);
	return match;
}

Wouldn't that retain the current production behavior, but give more insight during development?

comment:7 Changed 5 years ago by Adam Peller

no, the specified behavior is to throw an exception. you could do something like

if (has('config-isDebug') && value === undefined) {
	console.warn(className + ' template couldn\'t find: ' + key);
	throw new Error(className + ' template couldn\'t find: ' + key);
}

or even just

if (has('config-isDebug') && value === undefined) {
	throw new Error(className + ' template couldn\'t find: ' + key);
}

and wouldn't it belong outside the if (format) condition?

Last edited 5 years ago by Adam Peller (previous) (diff)

comment:8 Changed 5 years ago by dylan

I'm confused, where is the current code actually throwing an exception?

https://github.com/dojo/dojo/blob/master/string.js#L137-L148

comment:9 Changed 5 years ago by bill

It doesn't use the actual "throw" keyword; it throws a NullPointerException? or something like that, by executing illegal javascript. This is the same way most dojo code fails when you pass an illegal argument.

comment:10 Changed 5 years ago by ben hockey

i'm in favor of failing with a better error but we shouldn't change the behavior to silently swallow errors.

comment:11 Changed 5 years ago by dylan

Ok, then my previous point was what I thought, which is basically that I'm changing the behavior when you're in debug mode, to not basically throw an error and break execution because you've forgotten a string replacement value. The idea being, maybe you're not finished with your translations yet, you still get a console warning message rather than silently swallowing an error.

The current behavior is more brittle than other templating systems, which is why this issue was raised.

comment:12 Changed 5 years ago by Bryan Forbes

The one argument I've heard (and experienced) for "swallowing" errors is for when translators are translating an application. Sometimes translators might not be programmers and having a missing property on an i18n bundle could bomb the application. In this case, a non-programmer translator would have no clue why the application is breaking other that to open the dev console and see a type error (since there's no such thing as a NullPointerException?) and have no clue what's going on. With the proposed solution, the application would still work, but the translator would see "${i18n.someProperty}" in the application instead of a properly translated string.

comment:13 in reply to:  12 Changed 5 years ago by ben hockey

Replying to BryanForbes:

The one argument I've heard (and experienced) for "swallowing" errors is for when translators are translating an application. Sometimes translators might not be programmers and having a missing property on an i18n bundle could bomb the application.

my opinion is that an error is better in this case. a mistake was made - it should cause an error. finding "${i18n.someProperty}" at the end of a lengthy terms of service agreement in a 200px tall dialog and is only shown to new users is not likely to be obvious even if someone does bother to look at it. an error though, will draw attention to the missing translation.

not throwing an error in a place where one was previously intentionally thrown is a breaking change. it could be a stretch but maybe someone was using the thrown error to determine whether or not a user had filled in all the fields of a form or some similar situation - that wouldn't work with this change.

comment:14 Changed 5 years ago by Adam Peller

What Bryan said, plus automated testing tools, which translators should be leveraging, would also have a much easier time noticing an exception after exercising cases to expose the various bundle properties.

Divergent behavior at debug time is something that should be avoided -- huge headaches there -- especially if it violates the basic contract of the API.

the substitute() method was intentionally minimalist. If you really want to throw verbose errors or substitute default strings in your application, you can always use the transform option, e.g. [9629]

Version 0, edited 5 years ago by Adam Peller (next)

comment:15 Changed 5 years ago by bill

Agree w/@neonstalwart and @peller, and also wanted to mention that the whole complaint about missing translations is bogus because when you have a missing translation (for example, the Japanese version of "OK"), the i18n code defaults back to the root bundle, getting the string in the original language (typically English).

comment:16 Changed 5 years ago by dylan

Ok, I see one of two options forward for this one:

  1. Reject
  1. Keep the original behavior, unless you use an opt-in flag, e.g.:
if (has('string-substitute-warn') && value === undefined) {
	console.warn(className + ' template couldn\'t find: ' + key);
	return match;
}

I know that in at least one project, we have done something like this, so maybe Bryan Forbes will give some feedback when he has time.

comment:17 Changed 5 years ago by dylan

Revised pull request to make this opt-in at https://github.com/dojo/dojo/pull/132

comment:18 Changed 4 years ago by dylan

Resolution: wontfix
Status: newclosed

Decision was made to reject this feature request for reasonable reasons.

Note: See TracTickets for help on using tickets.