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

Support i18next 22, react-i18next 12 and move both to peer-deps #1966

Merged
merged 29 commits into from Oct 26, 2022

Conversation

belgattitude
Copy link
Contributor

@belgattitude belgattitude commented Oct 19, 2022

Move react-i18next and i18next to peerDependencies and update them to latest

Why ?

  • If we want to support 2 major versions of i18next, that's the way.
  • react-i18next / next-i18next are sensitive to duplicates (two instances), peer deps will allow the consuming app/package to decide and will help deduplication by the package manager.

Note
Not all problems about instances are related to deduplication... next-i18next and react-i18next are prone to dual packaging hazards. I'll plan to provide a fix for that in a later iteration (on both packages). But moving to peer-deps will help as a first step. Cause I feel it's difficult to reason when fixing issues (ie: #1917)

What's new

  • Yarn PNP should work (need to confirm)

Version

  • Major (v13) to clearly indicates the change in install
  • No breaking changes

Makes sense to merge along #1974 and possibly

Upgrade

See https://github.com/i18next/next-i18next/pull/1966/files#diff-f411a5009131db5a6c7242b49ca78488f2d274ef9e5e5f4de9262a42108a5386

Notes

Recent versions of packages managers finally aligned on peer-deps and does not install them by default.

In certain situations (auto-install-peers in pnpm or npm v7), some people might forget to add the peer-deps as documented in v13. That might lead to invalid bug reports if the auto-installed version is not within the expectations or created legit duplicates (semver). In those possible cases, ensure both react-i18next and i18next are installed in the consuming app.


PS: I couldn't run npm run test without doing npm run install:examples first.

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

@adrai
Copy link
Member

adrai commented Oct 19, 2022

Let's wait for the react-i18next update first

@belgattitude
Copy link
Contributor Author

@adrai react-i18next use peerDependencies with a >=19.0.0 range.

https://github.com/i18next/react-i18next/blob/a0eed42303bda4863a832cbd8b280d04448f2690/package.json#L92

So technically speaking there's nothing on their side. That said I've just forced the ^22.0.0 on this repo to test it out:

belgattitude/nextjs-monorepo-example#2802

So far it passes e2e and previews.

@belgattitude
Copy link
Contributor Author

belgattitude commented Oct 19, 2022

That said I feel it would make sense to add i18next as a peerDependency at some point:

  "peerDependencies": {
    "i18next": "^21.9.1 || ^22.0.0"
  }

Plus update install instructions, ie: yarn install next-i18next i18next + maybe major to clarify the change

@adrai
Copy link
Member

adrai commented Oct 19, 2022

That said I feel it would make sense to add i18next as a peerDependency at some point:

  "peerDependencies": {

    "i18next": "^21.9.1 || ^22.0.0"

  }

Plus update install instructions, ie: yarn install next-i18next i18next + maybe major to clarify the change

There's an ongoing discussion about this: #1951

@belgattitude belgattitude changed the title Update to i18next ^22.0.0 Support i18next ^22.0.0 and move i18next and react-i18next to peer-deps Oct 20, 2022
@adrai
Copy link
Member

adrai commented Oct 20, 2022

@bryantobing12 @yuuk @martinjlowm @bojackhorseman0309 @rikkit @kachkaev @capellini @matthewwolfe @isaachinman can you please have a look at this PR?
Based on the discussion here: #1917 (comment)
My main concern is this PR conflicts with the current drop-in wrapper approach and probably will not solve all mentioned issues.

package.json Outdated Show resolved Hide resolved
@belgattitude
Copy link
Contributor Author

belgattitude commented Oct 20, 2022

@adrai I've looked and provided a fix for #1917 (comment). Tested with yarn and pnpm (see ithub.com/bryanltobing/layout-reproduce-i18next/pull/1).

With it I'm somehow letting the bundler choose the right package (not totally correct but I can't explain it more). That will circumvent dual package hazards. But at a later point the best is to rework the packaging/exports of both next-i18next and react-i18next. It might come with a major as well.

I'll take some time tomorrow to think if it's worthy to make both changes together.

@belgattitude
Copy link
Contributor Author

belgattitude commented Oct 20, 2022

In reply to

Based on the discussion here: #1917 (comment)
My main concern is this PR conflicts with the current drop-in wrapper approach

What we could say for sure

  • Only peer-deps will allow ranges (useful for i18next 21 && 22).
  • We loose drop-in approach (install 3 packages - I mean react, emotion... did it as well for reducing the amount of issues)

and probably will not solve all mentioned issues.

Not cause when going into detail, some of them comes from the dual packaging (try to play by removing the cjs or esm export)... But at least we can distinguish problems related to duplicates and exclude this scenario (and noisy debugging). Package managers (and their versions) aren't equal regarding duplicated

@adrai
Copy link
Member

adrai commented Oct 20, 2022

  • We loose drop-in approach

no problem for me, just want to be sure this is generally accepted...
see this discussion here: #136

@belgattitude
Copy link
Contributor Author

no problem for me, just want to be sure this is generally accepted...

Yes, true. My feeling: not everything is perfect. Some time ago I had the impression that most of the time, it ended up by a package manager discussion. Now they aligned. I've added a note in the description.

@belgattitude belgattitude mentioned this pull request Oct 20, 2022
@rikkit
Copy link

rikkit commented Oct 20, 2022

Happy to see the peer deps change looks like it's going ahead, I'll close my PR #1951 since this branch has the same changes.

@belgattitude I'd be happy to test this branch using Yarn 3 PnP if you need, let me know.

@belgattitude
Copy link
Contributor Author

belgattitude commented Oct 20, 2022

@rikkit thanks for for proposal. Out of my mind, if you use yarn you can take this

bryanltobing/layout-reproduce-i18next#1

Run yarn patch next-i18next, move the deps to peer-deps as in this P/R.

Change the .yarnrc.yml to to pnp linker, nuke the node_modules and run install.

@belgattitude belgattitude changed the title Support i18next ^22.0.0 and move i18next and react-i18next to peer-deps Support i18next ^22.0.0 and move i18next and react-i18next to peer-deps (do not merge) Oct 20, 2022
@adrai
Copy link
Member

adrai commented Oct 26, 2022

@belgattitude fyi: #1991

# Conflicts:
#	examples/simple/package.json
#	examples/ssg/package.json
@adrai
Copy link
Member

adrai commented Oct 26, 2022

fyi: #1982 (comment)

@adrai
Copy link
Member

adrai commented Oct 26, 2022

fyi2: there might still be an issue if using an i18next backend to load the resources in combination with TS: i18next/i18next#1853 (comment)

@belgattitude belgattitude changed the title Support i18next 22, react-i18next 12 and move both to peer-deps (do not merge) Support i18next 22, react-i18next 12 and move both to peer-deps Oct 26, 2022
@belgattitude
Copy link
Contributor Author

belgattitude commented Oct 26, 2022

@adrai thanks for the links, I won't have time to read them properly today. But on my limited time, I would propose this t(o ease the process).

  1. Review this PR in order to merge it. Otherwise I have to deal with too much context shift and risk to mess up 😄
  2. Merge it (so I can make it easier to work on both the i18next types augmentation in there Refactor example types to i18next v22 (do not merge) #1994. and the new exports in Initial support for es module exports  #1973)

This PR scope is:

  • Move to peer
  • Update to latest

That way we work on step-by-step basis and fix along the way till v13 is in ready state

@adrai
Copy link
Member

adrai commented Oct 26, 2022

Since there was not much feedback regarding the peer deps, I think we should merge it... (but not yet release it)
Can you update the i18nest-fs-backend dependency before merging? This would solve also this issue (#1982), right?

@adrai
Copy link
Member

adrai commented Oct 26, 2022

Since there was not much feedback regarding the peer deps, I think we should merge it... (but not yet release it) Can you update the i18next-fs-backend dependency before merging? This would solve also this issue (#1982), right?

updating your branch with master and updating i18next-fs-backend to v2.0.0 in your branch should already be ok

@belgattitude
Copy link
Contributor Author

Great ! v2.0.0 updated.

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

5 participants