Be explict about some i18n oddness around undefined keys, and TextField's value #2598
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BPScott
changed the title
BPScott
force-pushed
the
explict-i18n-oddness
branch
from
f28c066 to
e013624
Compare
dleroux
reviewed
Jan 8, 2020
| // Use a typeof check here as Typescript mostly protects us from non-stringy | ||
| // values but overzealous usage of `any` in consuming apps means people have | ||
| // been known to pass a number in, so make it clear that doesn't work. | ||
| const normalizedValue = typeof value === 'string' ? value : ''; |
dleroux
reviewed
Jan 8, 2020
src/utilities/i18n/I18n.ts
Outdated
| // for now as JS can handle it ok. We should work out how to be | ||
| // stricter, or deliberatly allow undefined as a value | ||
| return replacements[replacement] as string; | ||
| return replacements[replacement].toString(); |
dleroux
reviewed
Jan 8, 2020
| `No replacement found for key 'string'. The following replacements were passed: 'notString'`, | ||
| `Error in translation for key 'key'. No replacement found for key 'string'. The following replacements were passed: '{"notString":"bar"}'`, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Not 100% necessary but it might be best to split these 2 expect statements into 2 tests?
BPScott
force-pushed
the
explict-i18n-oddness
branch
from
e013624 to
d263c54
Compare
Improve the error message, and have the error trigger if you have
`{foo: undefined}`
This ensures that normalizedValue is always a string - previously it could be a number if you forced it through with `any` and that caused problems further down the line
BPScott
force-pushed
the
explict-i18n-oddness
branch
from
d263c54 to
29e86d1
Compare
|
A bit or rebasing to split these changes into isolated commits |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Following up #2579.
WHAT is this pull request doing?
Make passing
{foo: undefined}into translations replacements a runtime error, in addition to a TS type error for those fun cases where people power through mistyping things usingany(looking at you enzyme's trigger function that isn't type-safe).Make TextField's
normalizedValuebe an empty string if you pass anything other than a string into it. Once again this is limited to be a string only by typescript but people using those not-type-safe functions can pass in a number and not be warned. Having normalizedValue be a number mucks up a bunch of things so catch that at runtime too.Make TextField a little lazier by only working out character count stuff if we're actually about to display it.
How to 🎩
tests pass