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

Move i18next and react-i18next to peerDependencies #1951

Closed
wants to merge 1 commit into from

Conversation

rikkit
Copy link

@rikkit rikkit commented Sep 15, 2022

This PR moves i18next and react-i18next to peerDependencies, so that consuming Yarn PnP projects aren't forced to have multiple instances of i18next if they themselves depend on react-i18next.

After this PR, projects that use next-i18next will have to ensure they have react-i18next and i18next in their own dependencies:

npm i react-i18next i18next
yarn add react-i18next i18next

See discussion in #1917

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

…uming Yarn PnP projects don't have multiple instances of i18next

See i18next#1917
@stale
Copy link

stale bot commented Sep 24, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 24, 2022
@rikkit
Copy link
Author

rikkit commented Sep 26, 2022

@Stale not stale!

@stale stale bot removed the stale label Sep 26, 2022
@stale
Copy link

stale bot commented Oct 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 12, 2022
@rikkit
Copy link
Author

rikkit commented Oct 17, 2022

not stale

@stale stale bot removed the stale label Oct 17, 2022
@matthewwolfe
Copy link

I unfortunately don't see this getting merged @rikkit 😞

@rikkit
Copy link
Author

rikkit commented Oct 18, 2022

I am happily using this via the @rikkilt/next-i18next package so know it works... without this change this library is not compatible with any package manager that does not use node_modules to link packages i.e. Yarn in PnP mode or pnpm.

I guess I will just maintain that separate package for the forseeable.

@belgattitude
Copy link
Contributor

belgattitude commented Oct 19, 2022

Per comment here: #1966 (comment)

I'd suggest to move to peer-deps. Why ?

@adrai if you feel it's a good move, I'll update my P/R about i18next update: #1966.

I also suggest to not use ranges like >= ...

Let me know

@adrai
Copy link
Member

adrai commented Oct 19, 2022

@belgattitude the problem is, this: #1917
especially: #1917 (comment)

@belgattitude
Copy link
Contributor

belgattitude commented Oct 19, 2022

Thanks, I'll take time to read them properly soon.

I've run into similar issues in the past 2 years... Most of the time it was caused by duplicates // react-i18next/i18next and less by legit next-i18next edges cases

For install issues I could fix either by

  • forcing resolutions
  • or by deduping (yarn-deduplicate / yarn 2+ built-in dedupe / pnpm-deduplicate...). Deduping have limits of semver of course (ie recent i18next ^22)

A problem that peer-deps would avoid as the consuming app has the control.

Some popular react/context libraries even moved to peer-deps for the deduplication problem (even if they don't need multi-ranges): ie emotion...

I haven't created a bug report about that and haven't followed so much. But about the links you've provided to read, I would recommend to gather

Node/package manager

cd /the-repo-or-monorepo
npx envinfo

Plus

cd /the-repo-or-monorepo

pnpm -r why i18next react-i18next next-i18next

// or 

yarn -R why i18next 
yarn -R why react-i18next 
yarn -R why next-i18next

// or...

Cause otherwise I feel it's difficult/time-consuming to know/distinguish between install issues (node resolution) and nextjs/next-i18next issues. In my experience there's issues in both of them, but much easier to isolate

I'll try to have look.

Thanks a lot

@adrai
Copy link
Member

adrai commented Oct 20, 2022

Thanks, I'll take time to read them properly soon.

I've run into similar issues in the past 2 years... Most of the time it was caused by duplicates // react-i18next/i18next and less by legit next-i18next edges cases

For install issues I could fix either by

  • forcing resolutions

  • or by deduping (yarn-deduplicate / yarn 2+ built-in dedupe / pnpm-deduplicate...). Deduping have limits of semver of course (ie recent i18next ^22)

A problem that peer-deps would avoid as the consuming app has the control.

Some popular react/context libraries even moved to peer-deps for the deduplication problem (even if they don't need multi-ranges): ie emotion...

I haven't created a bug report about that and haven't followed so much. But about the links you've provided to read, I would recommend to gather

Node/package manager

cd /the-repo-or-monorepo

npx envinfo

Plus

cd /the-repo-or-monorepo



pnpm -r why i18next react-i18next next-i18next



// or 



yarn -R why i18next 

yarn -R why react-i18next 

yarn -R why next-i18next



// or...

Cause otherwise I feel it's difficult/time-consuming to know/distinguish between install issues (node resolution) and nextjs/next-i18next issues. In my experience there's issues in both of them, but much easier to isolate

I'll try to have look.

Thanks a lot

@kachkaev @capellini can you provide feedback?

@rikkit
Copy link
Author

rikkit commented Oct 20, 2022

Closing in favour of #1966

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.

4 participants