Make sure key in antl.formatMessage() is a string before splitting, return default value if not #37
Conversation
…(), return default value if key is not splittable
|
Should we throw an exception instead of returning null when the key isn't a string? What do you think @thetutlage? |
|
Yeah returning |
| * Make sure key is a string before attempting to split it | ||
| */ | ||
| if (typeof (key) !== 'string') { | ||
| return defaultValue |
There was a problem hiding this comment.
We need to throw an exception instead of returning the default value.
There was a problem hiding this comment.
An empty string returns the default value, so would it be acceptable to return the default value if the key is null or undefined and only throw an exception if the key is not a string/undefined/null?
This would keep people from having to guard against null or replace it with "" in every case where formatString() is used on a variable containing a key.
There was a problem hiding this comment.
If you don't have a key, then you shouldn't be calling this method at all. Guarding against these sort of use cases is not a good practice, the application code to handle the inconsistencies itself.
Also I am not sure how come you can have the defaultValue but don't have the key?
There was a problem hiding this comment.
I was counting formatValue()'s default of null (via get()'s default) as the defaultValue in that case.
The suggested change makes sense from the point of view of having the user do all of the guarding logic in application code. I was missing that context and tried to fill it in from other behavior, specifically what happens with an empty string key.
If the lack of a key is intended to be an error case, it seems like an empty string key should also throw an exception. Is that correct?
Currently, if you provide an empty string key, you will get the defaultValue back due to the use of _.get() with both group and messageKey being null. I added a test to confirm this behavior locally.
I just want to make sure this ends up behaving as intended when I make the changes.
There was a problem hiding this comment.
Definitively better to throw an exception there. Could you make the change?
Proposed changes
Check to make sure that the key being passed to antl.formatMessage() is a string before using .split() on it in order to prevent errors/exceptions trying to use a string function on a non-string.
If the key is not a string, return the default value. This seems appropriate given the fact that if the key is not found, the default is returned.
Types of changes
What types of changes does your code introduce?
Checklist
Further comments
This is a relatively small change to fix a bug and avoid having to try/catch around instances of antl.formatMessage() in cases where the key may be sourced from a variable.