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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor generics usage #1180

Merged
merged 14 commits into from Jan 23, 2019

Conversation

Projects
None yet
5 participants
@rosskevin
Copy link
Collaborator

rosskevin commented Jan 22, 2019

BREAKING type change for those using generics and importing/reusing types. Typical usage in TS without importing TranslateFunction or generics should remain unaffected.

This fixes a regression introduced by #1163 and related PRs, but in turn disables use of generics in i18next.t<> pattern of usage. - see #1180 (comment)

As I searched for a solution to this mock test, after a suggestion below by @VincentLanglet I noticed we would be moving towards duplication. As I attempted to solve this, I think (due to duplication), I believe I ran into #1175 (comment).

In order to solve this, since I needed to remove duplication and alter potential usage of parameterized arg order, the easiest way to signal this to TS users is to change the exported name so that they will intentionally break and need to review their usage (sorry everyone 馃憢 - seems like the best option)

  • TranslateFunction -> TFunction
  • TranslateOptions -> TOptions
  • WithT.t - removed definition and extracted to TFunction
  • exists - stopped (mis)using TranslateFunction and defined separately
  • reorder the TFunction args to make easier for typical use (result first)
  • default TFunction to return string by default. For arrays and objects, parameterize the call e.g. t<object>('foo')

Mock test initiated by i18next/react-i18next#684 @nikolay-borzov

The mock should have been simple - but it wasn't.

rosskevin added some commits Jan 22, 2019

@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 23, 2019

@rosskevin I saw that

First of all, I think as expected the error from

const mockWithT: i18next.WithT = {
  t: (key: string) => key,
};

It should be

const mockWithT: i18next.WithT = {
  t: (key: string | string[]) => key,
};

But It still does not fix the error. And I actually don't know why

rosskevin added some commits Jan 23, 2019

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

Agreed @VincentLanglet - I tried a variety of things - none of which worked. The specific error from what you mentioned is:

test/typescript/mock.test.ts:7:3 - error TS2322: Type '(key: string | string[]) => string | string[]' is not assignable to type '<TKeys extends string = string, TValues extends object = object, TResult extends string | object | (string | object)[] = string | object | (string | object)[]>(key: TKeys | TKeys[], options?: TranslationOptions<TValues> | undefined) => TResult'.
  Type 'string | string[]' is not assignable to type 'TResult'.
    Type 'string' is not assignable to type 'TResult'.

7   t: (key: string | string[]) => key,
    ~

  index.d.ts:479:5
    479     t<
            ~~
    480       TKeys extends string = string,
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ...
    488       options?: TranslationOptions<TValues>,
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    489     ): TResult;
        ~~~~~~~~~~~~~~~
    The expected type comes from property 't' which is declared here on type 'WithT'

I normally parameterize the interface and use in the method. I haven't tried that because I don't see how that would make a difference but perhaps I should. @tkow - thoughts since you introduced this originally?

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

BTW - the diff is now cleaned up and much simpler.

@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 23, 2019

@rosskevin I get no error this way

interface TFunction<
  TKeys extends string = string,
  TValues extends object = object,
  TResult extends string | object | Array<string | object> =
    | string
    | object
    | Array<string | object>
> {
  (key: TKeys | TKeys[], options?: TranslationOptions<TValues>): TResult;
}

interface WithT {
  t: TFunction;
}

const mockWithT: WithT = {
  t: (key: string | string[]) => key,
};

@rosskevin rosskevin changed the title TS cleanup/add mock test Add mock test Jan 23, 2019

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

Given that @VincentLanglet - I think we need to make a type-breaking change and just update the TranslationFunction because your TFunction duplicates it with diff parameterized arg order. I'll make an attempt.

@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 23, 2019

@rosskevin I Agree

TranslateFunction -> TFunction
TranslateOptions -> TOptions
WithT.t - removed definition and extracted to TFunction
exists - stopped (mis)using TranslateFunction and defined separately

@rosskevin rosskevin changed the title Add mock test Refactor generics usage Jan 23, 2019

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

Ok, I think I have cleaned it up, though the generics test fails here:

i18next.t<string, KeyList>('friend', { myVar: 'someValue' });
i18next.t<string, KeyList>(['friend', 'tree'], { myVar: 'someValue' });
i18next.t<string, KeyList, { myVar: 'someValue' }>('friend', { myVar: 'someValue' });
i18next.t<string, KeyList, { myVar: 'someValue' }>(['friend', 'tree'], { myVar: 'someValue' });

Expected 0 type arguments, but got 2.
Expected 0 type arguments, but got 2.
Expected 0 type arguments, but got 2.
Expected 0 type arguments, but got 2.

Thoughts? Please check the updated description for a summary. I am tempted to comment out this test until @tkow has time to take a look. I think it is an important improvement that I think others may be running into.

@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 23, 2019

@rosskevin Indeed, this way we lose the ability to use t<A,B,C>(...)
@tkow or @Jessidhia will be better help than me

Since the PR of tkow introduced a regression, I understand you comment out theses tests, in order to fix it. We will search how to have a better t function after.

rosskevin added some commits Jan 23, 2019

duplicate TFunction definition inside WithT - can鈥檛 find a way around鈥
鈥 it - breaks mock.test but works for typical usage
@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 23, 2019

There is still a debate about the choice

No breaking change, but not classic

interface TFunction<
  TResult extends string | object | Array<string | object> =
    | string
    | object
    | Array<string | object>,
  TValues extends object = object,
  TKeys extends string = string
> {
  (key: TKeys | TKeys[], options?: TranslationOptions<TValues>): TResult;
}

Versus

Breaking change, but classic

interface TFunction<
  TKeys extends string = string,
  TValues extends object = object,
  TResult extends string | object | Array<string | object> =
    | string
    | object
    | Array<string | object>
> {
  (key: TKeys | TKeys[], options?: TranslationOptions<TValues>): TResult;
}
@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

@VincentLanglet the goal has always been to support classic usage. I just added some to t.test to demonstrate expected/typical use:

// various returns <string> is the default
i18next.t('friend');
i18next.t<object>('friend');
i18next.t<string[]>('friend');
i18next.t<object[]>('friend');

This should be the simplest use I think.

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

The current push works for everything except the mock - back to square one but with an improved basis. I strangely have to duplicate TFunction inside WithT to retain parameterized type usage with i18next.t<>. So close...

@VincentLanglet

This comment has been minimized.

Copy link
Contributor

VincentLanglet commented Jan 23, 2019

@rosskevin When @tkow made his PR, it was intented to be used this way I think. But it's debatable

i18next.t('friend');
i18next.t<'friend'>('friend');

Anyway, the actual real problem is that we are not able to type correctly in order to having both generic type and the test for the mock working. We're doing something wrong... but hard to find what.

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

Duplication removed, change init test to rely on inference instead of explicit types for callbacks, still have the mock issue.

This works now without explicit types (unless you want the return type of object or string[]). For parameterized TKeys use, you now have to use the second arg, which makes sense because most won't do that.

If I can get a good solution to the mock test, I think this is good to go.

rosskevin added some commits Jan 23, 2019

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

Added a workaround for the mock.test for the time being and filed an issue.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 23, 2019

Coverage Status

Coverage decreased (-0.3%) to 88.155% when pulling 921ae23 on rosskevin:ts-mock-withnamespaces into 5134e75 on i18next:master.

@rosskevin rosskevin merged commit 8944d0f into i18next:master Jan 23, 2019

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.3%) to 88.155%
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Jan 23, 2019

@jamuhl I need a release for this. This is a breaking typescript change (see description) - it primarily fixes a regression introduced but to do so had to change order of parameterized args. The best way to not silently cause a lot of confusion for TS users is to break it in very obvious way - change the name of the interfaces that were affected.

A major bump would make sense for TS users, but not for js users - your call on versioning. If you aren't worried, I would major bump it.

I will have a follow-up PR/release to react-i18next related to this.

@rosskevin rosskevin deleted the rosskevin:ts-mock-withnamespaces branch Jan 23, 2019

rosskevin added a commit to rosskevin/react-i18next that referenced this pull request Jan 23, 2019

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Jan 23, 2019

@rosskevin published in i18next@14.0.0

rosskevin added a commit to i18next/react-i18next that referenced this pull request Jan 23, 2019

TS generics fixes related to i18next/1180 (#691)
* TS generics fixes related to i18next/1180

i18next/i18next#1180

* Bump i18next dependency to needed 14.0.0
@janhartmann

This comment has been minimized.

Copy link

janhartmann commented Feb 5, 2019

I just updated my dependencies and apperently I am not able to do this anymore:

import { WithNamespaces, withNamespaces } from "react-i18next";

export interface IDurationProps extends WithNamespaces {
 /* other props */ 
}

const Duration: React.FC<IDurationProps> = ({ t }) => { 
   return <p>{t("my-key")}</p>
}

export default withNamespaces()(Duration);

My test:

describe("Duration", () => {
  const props: IDurationProps = {
    duration: 1800000,
    size: "medium",
    i18n: null,
    tReady: true,
    t: (key) => "h [hrs] mm [min]" // <-- Error: Compile Error!
  };

  /* omitted */

Type '"h [hrs] mm [min]"' is not assignable to type 'TResult'.ts(2322)
index.d.ts(473, 5): The expected type comes from the return type of this signature.

@k-schneider k-schneider referenced this pull request Feb 8, 2019

Open

Typescript issues #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment