-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
add generic type to t function and the tests #665
Merged
Merged
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
6bf2e16
add generic type to t function and the tests
tkow 0889c35
rollback test
tkow c2532aa
change: some omitted typing
tkow f82bf5f
add new test for generic translate function
tkow 77f66e2
add an example and override test
tkow 5b53253
chane inteface for TranslationFunction structured
tkow e277dca
change way to inherit type because of lacking of generic parameters.
tkow a789b47
add test example
tkow 276d34f
change peerDependencies to 13.1.3
tkow bb28274
add more example and make comments concise
tkow 09bfa0c
Merge pull request #1 from tkow/feature/add-traslate-generic
tkow a99940b
Merge branch 'master' of github.com:i18next/react-i18next into add-tr…
tkow 04997e7
adapt test new lint rules
tkow File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import * as React from "react"; | ||
import { | ||
NamespacesConsumer, | ||
Trans, | ||
withNamespaces, | ||
WithNamespaces, | ||
} from "../../src/index"; | ||
|
||
type TKeys = "title" | "text"; | ||
|
||
declare interface IWithNamespacesOverrideTest extends WithNamespaces { | ||
t<T extends string| string[]>(a: T): any; | ||
} | ||
|
||
function NamespacesConsumerTest() { | ||
return ( | ||
<NamespacesConsumer> | ||
{(t, { i18n }) => | ||
<div> | ||
<h2>{t<TKeys>("title")}</h2> | ||
<span>{t("any")}</span> | ||
<span>{t("any", {anyObject: {}})}</span> | ||
<span>{t<TKeys>("text")}</span> | ||
<span>{t<TKeys, {key: string}>("text", {key: "foo"})}</span> | ||
</div> | ||
} | ||
</NamespacesConsumer> | ||
); | ||
} | ||
|
||
function TransComponentTest({ t }: WithNamespaces) { | ||
return ( | ||
<div> | ||
<Trans i18nKey="description.part1"> | ||
To get started, edit <code>src/App.js</code> and save to reload. | ||
</Trans> | ||
<Trans i18nKey="description.part1"> | ||
To get started, <strong title={t<TKeys>("title")}>{{name}}</strong>and save to reload. | ||
</Trans> | ||
</div> | ||
); | ||
} | ||
|
||
const MyComponentWrapped = withNamespaces()(TransComponentTest); | ||
|
||
type ArticleKeys = "article.part1" | "article.part2"; | ||
type AnotherArticleKeys = "anotherArticle.part1" | "anotherArticle.part2"; | ||
class App extends React.Component<WithNamespaces> { | ||
public render() { | ||
const { t, i18n } = this.props; | ||
|
||
const changeLanguage = (lng: string) => { | ||
i18n.changeLanguage(lng); | ||
}; | ||
|
||
return ( | ||
<div className="App"> | ||
<div className="App-header"> | ||
<NamespacesConsumerTest /> | ||
<button onClick={() => changeLanguage("de")}>de</button> | ||
<button onClick={() => changeLanguage("en")}>en</button> | ||
</div> | ||
<div className="App-intro"> | ||
<MyComponentWrapped /> | ||
</div> | ||
<article> | ||
<div>{t<ArticleKeys>("article.part1")}</div> | ||
<div>{t<ArticleKeys>("article.part2")}</div> | ||
</article> | ||
<article> | ||
<div>{t<AnotherArticleKeys>("anotherArticle.part1")}</div> | ||
<div>{t<AnotherArticleKeys>("anotherArticle.part2")}</div> | ||
</article> | ||
</div> | ||
); | ||
} | ||
} | ||
export default withNamespaces("translation")(App); |
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.
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.
Can we have something more specific than any for TResult ?
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.
It was migrated to i18next https://github.com/i18next/i18next/blob/master/index.d.ts#L463-L480 .
Default value is set any before I imprement tha,but I understood TrResult may be more strict as you mean.
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.
I agree default value was set any before your implementation. But since you're working on the generic, it was perfect time to follow the boyscout rule. There is so much any in these typings.
Since this library is restricted for react use only, it's easier to have more strict type. Something like string
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.
@VincentLanglet I take a different tact. Incremental improvement.
any
is something and if you feel strongly about improving the type then PR it. Something is better than nothing, in this caseany
is better than no type at all so I don't pile on and ask for more changes that are near but not necessarily the intent. Otherwise it would spider into fixing allany
types. I encourage small focused PRs if possible. Review/test/merge them quickly and let everyone contribute to their concern.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.
I'm not talking about fixing all any types. Just the one he's introducing.
You ask for my contribution, but this code isn't already in the base code.
When we write code, we write test too, we don't explain "Someone can do the test for me". It's the same here.
I don't file it's difficult to change TResult = any by something like TResult extends string = string.
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.
@rosskevin I have no worry to make a PR, and I did : i18next/i18next#1175
I waiting review to check what is the best type to use.
But it's not a good idea to accept every changes of code with a fake rule of "Incremental improvement". On the contrary, it's the best way to add bugs or worse, technical debt.
In the previous PR, the type introduced could be more specific and even has typos in the commentary. The best is the enemy of good, but always ask yourself if what you're doing is really good.
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.
Thanks @VincentLanglet - you just proved my point and incrementally improved the project!