Skip to content
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 13 commits into from Jan 18, 2019

Conversation

@tkow
Copy link
Contributor

tkow commented Jan 8, 2019

Closes #662

  • change t function arguments' type any to generic
  • add tests for generic type is available to
    test/typescript/examples-react.test.tsx
    test/typescript/NamespacesConsumer.test.tsx
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 8, 2019

Coverage Status

Coverage remained the same at 75.157% when pulling 04997e7 on tkow:add-traslate-generic into aa71b53 on i18next:master.

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Jan 9, 2019

@rosskevin would love to get a ok/nok from you before merging this!

@jamuhl jamuhl added the typescript label Jan 9, 2019
Copy link
Collaborator

rosskevin left a comment

Please leave existing test assertions as-is because it represents typical usage. Please add a new test that shows you can also use it parameterized.

Based on my reading of this change, it will not work for typical inputs and will require manual definition of types.

As is this PR should not be accepted.

@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 9, 2019

@rosskevin

Thanks for review.

it will not work for typical inputs

What do you mean that? I haven't have no idea about the not working case, though I may miss something.

In previous definitions, all typeParameters used Default, thus had disfunction of parametric type, so I only extended to enable the type customized including interface is overridable about t function, so these change should not include any degradation for all default params referred from i18next.TranslationFunction types, though no type-safe restraint in default as it has been same ever.

I want I18nContextValues to overridable feature, because I use redundant type definition as I described in #662.

But if you don't want to accept this, it's okey to me, I don't have trouble now in this matter.
Please tell me, should I change my tests or close this.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 9, 2019

@tkow - very happy to accept improvements but we have to accommodate those that do not want to force parameterization onto all typescript users - so you have to make sure use of parameterized types is optional.

  • First revert the current test to prove that your improvement works for everyone
  • Second add a new test without changing the current one to show that parameterization works
@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 9, 2019

Thanks for response so quickly. I add some improvements. Please check anytime when you have time,

Copy link
Collaborator

rosskevin left a comment

Thanks for making those changes. I have a clearer picture now.

My real question about this PR is does it add value to static analysis? Are you auto-discovering keys to generate your own TKeys, or are you manually specifying them as you show in the sample test? If you are manually specifying them like the test shows, it is not adding value as you are just shifting a potential mistake from a t() call to a type TKeys = 'title' | 'text' definition. Both are equally susceptible to mistake in the same exact way. I think we should first address this question before going further. Unless there is a way to auto discover keys, static analysis is not improved with this PR.

To summarize, here are my two concerns:

  1. react-i18next types are now disconnected from i18next. If parameterization proves useful, it looks like it needs to start with a PR in i18next and be used here. That would eliminate the ReactI18nextTranslateFunction
  2. The types specified are manual and not discovered so it will not add value to static analysis (unless there is a method on your side of which I am unaware)
@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 9, 2019

I apprecate many feedback. In fact, I made a generator union types from json keys.

https://github.com/tkow/key2union

So we use TranslateKeys previous issue says from other generators.

I understood what you said.
I want to reply asap, but I'll do tommorrow because midnight in my contry.

The pr most merit is to make overridable interface t function as original union types using merge interface rules. Then we can write less external module. So make this good a little.

Thanks for a lot of your time

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 9, 2019

Nice, glad to see you are working to improve your types - gives me some ideas!

So I think we should pursue this, but first I think you should submit a parameterized PR for the types in i18next which will allow you to remove the ReactI18nextTranslateFunction and use a generic version of i18next.TranslationFunction

@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 10, 2019

@rosskevin I pull-requested first i18next/i18next#1163 and check it, this has no breaking compatibility. After merging it, I'll rewrite this definition file using i18next.translateFunction, it' ll be more simple than first one. Thanks for your shrewd advice.

(Edit) : I had many investigation and found, if we want to input generic typed parameters for props, we may have to re-define short-hand prop type from TranslateFunction

inteface  A{
  t<A=default,B=default,C=default>(a:A,b:B):C;
}

because prop t don't acquire parametric ability. This modification can be ugly (though that is useful because simply give unions first TypeParameter when using it, for someone don't get used to amibient file)
But, I noticed that if t type's interface, we can override inherit prop though can't about arrow function.
So,

see examples

This doesn't need change almost nothing about this repository though we need manual override this module.

Please give me opinion , Anyway change about i18next/i18next#1163 is not useless whichever we take approach.

Interestingly , at latest typescript may inherit generic params dynamical typing with using typeof though my local 2.9.3 typescript with error as same statements.

see the test B example.

@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 10, 2019

@rosskevin
Can I make new WithT interface and export it from i18next?
This is probably most simple implementation both generic parametrization and overridable feature.
I'll separate pull-requests, function interfaces change merge first , second add interface WithT Interface in i18next repository. Last, I change this PR using extends i18next.WithT and i18next.TranslateFunction only.

import * as React from "react";
import i18next from "i18next";

export interface I18nContextValues extends i18next.WithT {
  i18n: i18next.i18n;
  defaultNS?: string;
  reportNS?: string;
  lng?: string;
}

export interface NamespacesConsumerProps extends ReactI18NextOptions {
  ns?: Namespace;
  i18n?: i18next.i18n;
  initialI18nStore?: {};
  initialLanguage?: string;
  children: (
    t: i18next.TranslationFunction,
    options: {
      i18n: i18next.i18n;
      lng: string;
      ready: boolean;
    }
  ) => React.ReactNode;
}

export interface TransProps extends Partial<i18next.WithT>{
  i18nKey?: string;
  count?: number;
  parent?: React.ReactNode;
  i18n?: i18next.i18n;
  defaults?: string;
  values?: {};
  components?: React.ReactNode[];
}

my implements is now just above.

@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 10, 2019

I've done this implements and passed my local test.
If my PR in i18next repo can be approved to merge, then I merge https://github.com/tkow/react-i18next/pull/1/files in my repo and reflect this PR.

@@ -14,6 +14,11 @@ interface ReactI18nextModule {
init: (instance: i18next.i18n) => void;
}

interface ReactI18nextTranslateFunction {
t<TKeys extends string=string,TVals extends object =object,TResult=any>

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet Jan 17, 2019

Contributor

Can we have something more specific than any for TResult ?

This comment has been minimized.

Copy link
@tkow

tkow Jan 17, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet Jan 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@rosskevin

rosskevin Jan 17, 2019

Collaborator

@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 case any 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 all any types. I encourage small focused PRs if possible. Review/test/merge them quickly and let everyone contribute to their concern.

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet Jan 17, 2019

Contributor

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.

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet Jan 17, 2019

Contributor

@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.

This comment has been minimized.

Copy link
@rosskevin

rosskevin Jan 17, 2019

Collaborator

Thanks @VincentLanglet - you just proved my point and incrementally improved the project!

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 17, 2019

@tkow i18next/i18next#1172 is merged and should be released soon so that will allow you to rework this based on your PR to i18next.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 17, 2019

@tkow i18next@13.1.3 is released.

tkow added 3 commits Jan 18, 2019
Rewrite interface with WithT interface introduced
@tkow tkow force-pushed the tkow:add-traslate-generic branch from 12aa64c to 04997e7 Jan 18, 2019
@tkow

This comment has been minimized.

Copy link
Contributor Author

tkow commented Jan 18, 2019

I have changed commits latest.
It will not be affected by i18next's next version bump.
Please check it.

@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 18, 2019

LGTM.
Plus, it will use this i18next/i18next#1175 as soon as it's patch.

@rosskevin rosskevin merged commit 450303d into i18next:master Jan 18, 2019
3 checks passed
3 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 75.157%
Details
@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 18, 2019

@jamuhl this can be a patch release

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Jan 18, 2019

published in react-i18next@9.0.4

@nikolay-borzov

This comment has been minimized.

Copy link
Contributor

nikolay-borzov commented Jan 21, 2019

With these changes, it's impossible to simply return key from t (e.g. when mocking)

export function getWithNamespacesProps(): WithNamespaces {
  return {
    tReady: false,
    t: (key, options) => key,
    i18n
  };
}

Type error: Type 'string | TKeys[]' is not assignable to type 'TResult'.
Type 'string' is not assignable to type 'TResult'. TS2322

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 21, 2019

@nikolay-borzov if you want to PR a breaking test, probably a dedicated file to this use-case, then I'll take a look and see if we need to adjust anything.

@nikolay-borzov

This comment has been minimized.

Copy link
Contributor

nikolay-borzov commented Jan 21, 2019

Sure #684

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.