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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TS migration] Migrate 'Localize' lib to TypeScript #29742
[TS migration] Migrate 'Localize' lib to TypeScript #29742
Changes from all commits
f2475fe
83a5aa4
ddcbd52
caadaeb
645650b
a477984
6d61ffd
c66ab48
e51c0f0
cc93605
79e367f
76ef27c
a63719c
d9d7df4
c352b53
7c68bde
1a569e4
30359af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translate function used to take 3 arguments and the third one being an object containing the parameters to be used by the translation phrase. Now the third argument is using the rest syntax and it's an array. Was that change intended?
Also previously the 3rd argument had a default value and now it does not which caused #34668 and #30949
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was an intended change, now this function can take 2 arguments for translations like:
3 arguments if translation takes additional argument:
4 and more arguments if translation takes more than one argument:
Translation functions that takes arguments should always receive them, so I think this just uncovered these bugs 😅
I think we should always add missing parameters to
translate
functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if we can have typescript help us with that? i.e. yield an error if we are missing to pass parameters to a translation function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already implemented @s77rt, but only in .ts/.tsx files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we casting
variables as never
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arosiclair We're casting
variables
asnever
to bypass TypeScript's strict type checking.translateIfPhraseKey
is being used dynamically, e.g., with errors that comes from the backend which we don't have static types for.It allows
translateIfPhraseKey
to call the strictly typedtranslateLocal
function without type errors. Yep, it does reduce type safety for variables, but looking at the implementation oftranslation
andtranslationLocal
it shouldn't be an issue.