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 ready/tReady type definitions #753

Merged
merged 9 commits into from
Mar 6, 2019
Merged

Conversation

traverse
Copy link
Contributor

Adds missing type definitions for the ready return value of the useTranslation hook and the tReady prop added by the withTranslation HOC, fixes #751.

Adds missing type definitions for the `ready` return value of the `useTranslation` hook and the `tReady` prop added by the `withTranslation` HOC.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.282% when pulling 5463e19 on traverse:patch-1 into 9042b09 on i18next:master.

Copy link
Collaborator

@rosskevin rosskevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add usage example to test/typescript to prevent regressions

@traverse
Copy link
Contributor Author

traverse commented Feb 25, 2019

@rosskevin do you mean something like the following in examples.test.tsx:

// Component that uses ready to wait until translations are loaded because useSuspense is set to false
function ComponentNotUsingSuspense() {
  const { t, ready } = useTranslation();
  return (
    <div>
      {ready && t('key1')}
    </div>
  );
}

// Class based component that uses tReady to wait until translations are loaded because useSuspense is set to false
class ClassComponentNotUsingSuspense extends React.Component<WithTranslation> {
  render() {
    const { t, tReady } = this.props;
    return <h2>{tReady && t('title')}</h2>;
  }
}

If there's anything that you want worded or named differently let me know.

@rosskevin
Copy link
Collaborator

@traverse you can add both of those, but be sure to exercise them (just add them inside a closure):

function suspenseUsage() {
    // components code here

   // exercise components code
   return (
        <div>
            <ComponentNotUsingSuspense />
            <ClassComponentNotUsingSuspense />
         </div>
    )
}

@traverse
Copy link
Contributor Author

traverse commented Feb 25, 2019

@rosskevin Ok will do, thanks!

@rosskevin rosskevin self-assigned this Feb 26, 2019
@jamuhl
Copy link
Member

jamuhl commented Mar 1, 2019

@traverse any update on this?

@traverse
Copy link
Contributor Author

traverse commented Mar 5, 2019

@jamuhl sorry about not giving an update on this, I was quite pressed for time last week and was bedridden over the weekend 🤒. I'm going to be spending some time on this today.

@traverse
Copy link
Contributor Author

traverse commented Mar 5, 2019

@rosskevin any idea why the CI build might be failing? The tests are running fine locally 🤔

@rosskevin
Copy link
Collaborator

rosskevin commented Mar 5, 2019

@traverse - it seems bizarre. I have tracked down dtslint to be relying on TypeScript@next, which might reveal the error on ci and not local. We are currently testing with the release of 3.3.3.

I'm looking into why dtslint is on next - because I believe that is a nightly build.

❯❯ npm list typescript                                                                                                                                                                                                                                            ✘ 1 
[email protected] /Users/kross/projects/react-i18next
├─┬ [email protected]
│ └── [email protected] 
└── [email protected] 

@rosskevin
Copy link
Collaborator

Issue microsoft/dtslint#159

@rosskevin
Copy link
Collaborator

@traverse this all looks good locally, I agree. I tried several things, all with full rebuilds, all which succeed locally but only fail on travis.

@jamuh I don't have much confidence in travis at this point. I checked and there are no caches configured so I'm not sure what else to do there. My assumption at this point is that any change in this repo (even non-ts) is going to fail on travis due to some environmental situation there.

I'm happy to merge this without travis success but worried that we will see more of the same. Thoughts?

@traverse
Copy link
Contributor Author

traverse commented Mar 5, 2019

@rosskevin I'm not sure if it would work in this case and if there's something similar for NPM but with Yarn you can use resolutions to overwrite specific dependency versions.

@rosskevin
Copy link
Collaborator

@traverse I tested with typescript next locally and it works, so it isn't a version conflict. Something on Travis.

@traverse
Copy link
Contributor Author

traverse commented Mar 5, 2019

@rosskevin It's also quite weird it's failing to find both withTranslation and WithTranslation but is finding useTranslation I'd assume it would either find none or all.

@jamuhl jamuhl merged commit 6f3ea70 into i18next:master Mar 6, 2019
@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2019

Same on circleci: https://circleci.com/gh/i18next/react-i18next/8

@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2019

Failed local at my place too: ef1ee56 fixes it

@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2019

@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2019

published in [email protected]

@traverse
Copy link
Contributor Author

traverse commented Mar 6, 2019

@jamuhl I think that #769 removed some things from examples.test.tsx maybe that's why it was failing in weird ways? It seems that way at least 🤔

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

Successfully merging this pull request may close these issues.

TypeScript type definitions hook/HOC aren't correct
4 participants