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 support to proper augment PluginOptions type #1583

Merged
merged 2 commits into from Mar 22, 2021

Conversation

pedrodurek
Copy link
Member

@pedrodurek pedrodurek commented Mar 21, 2021

This is the first PR that solves this problem: #1578
Another one will be created under i18next-locize-backend that depends on this.

It should have no breaking changes to users.

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • documentation is changed or added

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.057% when pulling 4bbbced on pedrodurek:master into a6e09e3 on i18next:master.

@adrai
Copy link
Member

adrai commented Mar 21, 2021

Would also other backends need to be updated?
like:

or other plugins like: https://github.com/i18next/i18next-browser-languageDetector ?

@pedrodurek
Copy link
Member Author

I'm going to take a look at the other repos.

@pedrodurek
Copy link
Member Author

Hey @adrai, let me know if there any remaining plugins.

@adrai
Copy link
Member

adrai commented Mar 21, 2021

Thank you for your help.
There probably are others... but if this is all there is to do... it's doable 😉

hope @jamuhl can have a look at it so we can create the appropriate releases...

If I've understood correctly this should not be breaking for neighter i18next nor the plugins, right?

@pedrodurek
Copy link
Member Author

No problem!!! Yeah, it should have no breaking changes on both sides

@adrai adrai merged commit a920877 into i18next:master Mar 22, 2021
@mrrotberry
Copy link

@pedrodurek , hmm, looks like you broke support of i18next-chained-backend, library for combine multiple of the existing backends( I'm talking about such a case:

i18nextInstance
  .use(I18NextChainedBackend)
  .init({
    backend: process.env.NODE_ENV === 'production'
      ? {
          backends: [LocalStorageBackend, I18NextHttpBackend],
          backendOptions: [localStorageBackendConfig, httpBackendConfig],
        }
      : {
          backends: [I18NextHttpBackend],
          backendOptions: [httpBackendConfig],
        },
    });

@pedrodurek
Copy link
Member Author

Hey @mrrotberry, yeah, It was reported yesterday, I'm on it! 😃

@mrrotberry
Copy link

OK, sorry didn't see 😃 could you tell me where this issue was discussed and where I could keep track of it?)

@pedrodurek
Copy link
Member Author

Sure! i18next/i18next-http-backend#58

@mrrotberry
Copy link

thank you very much and have a nice day)

@adrai
Copy link
Member

adrai commented Mar 25, 2021

@mrrotberry should be fixed with the new i18next-chained-backend version

@mrrotberry
Copy link

@adrai I confirm, the typing works fine, there are no errors. Thank you very much 👍

@adrai
Copy link
Member

adrai commented Mar 25, 2021

You have to thank @pedrodurek
he is the master of TypeScript 😉

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

Successfully merging this pull request may close these issues.

None yet

4 participants