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

Improve withSSR type definition #943

Merged
merged 2 commits into from Sep 15, 2019
Merged

Improve withSSR type definition #943

merged 2 commits into from Sep 15, 2019

Conversation

@skovy
Copy link
Contributor

skovy commented Sep 13, 2019

Currently, there are few issues with the withSSR type definitions:

  1. Function components aren't valid
  2. Any component (class or function) with props were invalid
  3. No type-safety around initialI18nStore and initialLanguage

Function Components

const FunctionComponent = () => {
  return null;
};

const ExtendedFn = withSSR()(FunctionComponent);
//                           ^^^^^^^^^^^^^^^^^
// Type '() => null' provides no match for the signature 'new (props: {}, context?: any): Component<{}, any, any>'

<ExtendedFn initialLanguage={'en'} initialI18nStore={{ en: { namespace: { key: 'value' } } }} />;

This is because the type definition for WrappedComponent is React.ComponentClass which doesn't include React.FunctionComponent. This can be solved with the React.ComponentType.

Class Components

class ClassComponentWithProps extends React.Component<{ foo: number }> {
  render() {
    return null;
  }
}

const ExtendedClassWithProps = withSSR()(ClassComponentWithProps);
//                                       ^^^^^^^^^^^^^^^^^^^^^^^
// Property 'foo' is missing in type '{}' but required in type 'Readonly<{ foo: number; }>'

<ExtendedClassWithProps
  initialLanguage={'en'}
  initialI18nStore={{ en: { namespace: { key: 'value' } } }}
  foo={1}
/>;

This is because the type definition for WrappedComponent is React.ComponentClass<{}, any> meaning only props of type {} are valid (a.k.a no props). This can be fixed with a generic Props.

initialI18nStore and initialLanguage

class ClassComponent extends React.Component {
  render() {
    return null;
  }
}

const ExtendedClass = withSSR()(ClassComponent);

<ExtendedClass initialLanguage={1234} initialI18nStore={5678} />;
//                              ^^^^                    ^^^^
//                             valid                   valid

From what I can infer from the docs and source, initialLanguage should be of type string. I am less confident about initialI18nStore but it appears it should be of type Resource? I'm basing this assumption off these types:

https://github.com/i18next/i18next/blob/85bbb39d66dc30ade0693c20076cbb4f84fd7652/index.d.ts#L650-L662

But in the code in this repo, it appears it's used as i18n.services.resourceStore.data = initialI18nStore; (and not i18n.services.resourceStore= initialI18nStore;) which these types suggest?

https://github.com/i18next/i18next/blob/85bbb39d66dc30ade0693c20076cbb4f84fd7652/index.d.ts#L672-L681

skovy added 2 commits Sep 13, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2019

Coverage Status

Coverage remained the same at 95.377% when pulling d5ee3ff on skovy:skovy/improve-wtih-ssr-types into 1a42148 on i18next:master.

@skovy skovy marked this pull request as ready for review Sep 13, 2019
@jamuhl jamuhl requested a review from rosskevin Sep 13, 2019
Copy link
Collaborator

rosskevin left a comment

To me it appears you have it correct and it is i18next.Resource. Let's go with this until we determine otherwise.

@rosskevin rosskevin merged commit ba954d2 into i18next:master Sep 15, 2019
4 checks passed
4 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
coverage/coveralls Coverage remained the same at 95.377%
Details
workflow Workflow: workflow
Details
@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Sep 15, 2019

This appears to be:

  1. fixing the ComponentType (more flexible - accepts Function components)
  2. removing the indexer and intersecting Props generic instead - more restrictive but correct and if it was being used properly will not break anything (though improper use will be revealed)
  3. tightening types on the initial* properties

TL;DR given the above I don't regard it as a breaking change, even though it is tightening types and some users may have new errors revealed.

Thanks @skovy

@jamuhl this can be a patch release.

@skovy skovy deleted the skovy:skovy/improve-wtih-ssr-types branch Sep 15, 2019
@skovy

This comment has been minimized.

Copy link
Contributor Author

skovy commented Sep 15, 2019

Thanks @rosskevin for the review, I appreciate it 🙌

@jamuhl

This comment has been minimized.

Copy link
Member

jamuhl commented Sep 16, 2019

published in react-i18next@10.12.5

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