aurelia / i18n

A plugin that provides i18n support.
MIT License
93 stars 70 forks source link

Fix for #295, upgraded i18next to v14.0.1 #296

Closed lcetinapta closed 5 years ago

lcetinapta commented 5 years ago

Upgraded i18next to v14.0.1 Changed all references of i18next.TranslationFunction to i18next.TFunction Changed all references of i18next.TranslationOptions to i18next.TOptions Removed compatibilityAPI from settings Changed i18next.init() to i18next.createInstance()

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

zewa666 commented 5 years ago

Hi @lcetinapta and thanks for your contribution. Can I ask you first to remove the dist/** files from this PR as they will get automatically generated when making a release. Your PR thus should only contain the actual 4 files you've modified. Second, I'm not sure whether this will result in a breaking change for aurelia-i18n users, since I haven't tested the new i18next version so far. We're pretty dependent on their changes and them creating a breaking change likely makes it to one for us as well. That said we'll need to give this one a bit more testing in order to decide what happens and maybe release it as a beta branch

lcetinapta commented 5 years ago

Hi @zewa666 , About the dist files... i reverted them back. Second, I agree there should be a beta branch for this, and I have no problem with this.

The problem is that there is no where mentioned that users of aurelia/i18n need to install speccific version of i18next (11.x version), the docs only mention to install i18next. So whoever installs i18next using the speccified command npm install i18next, will get the 14.0.1 version and aurelia/i18n will fail to build.

zewa666 commented 5 years ago

@lcetinapta fair statement. Since we didn't locked the dependency to a specific i18next version this indeed could be troublesome.

@EisenbergEffect what would be your idea on how to approach this? Merge that PR into a different branch and release a BETA of from that, until we're able to test everything out properly? This might not necessarily be a major bump, but nevertheless a beta could be a good idea.

zewa666 commented 5 years ago

Btw @lcetinapta thanks for cleaning up the PRed files :+1:

EisenbergEffect commented 5 years ago

It's a tricky situation with a critical dependency upping their major version. I'm inclined to also bump our major version in conjunction with that. We could do a beta release of that if you think that's required. Maybe make some more things explicit in the documentation as part of that too. I don't have any notion of whether it would actually affect someone though. How much do people use i18next directly when they are using our plugin?

zewa666 commented 5 years ago

@EisenbergEffect I can't really say that since features offered by i18next are huge. So I'd say it's a not a too huge amount but likely noticeable. If we'd go with the major bump should we then lock our dependency on a specific i18next version? Also with regards to the Beta, we had good experience when doing the port from JS to TS so I think another beta would be helpful to make an easier transition for people using this plugin.

Funny enough what @lcetinapta said is that he gets the version 14, so that sounds like we should also setup a peerDependency instead of simple dependency to make sure only the respecting branch gets in there.

Anyways @EisenbergEffect, if you're heading for a major bump that sounds like you'd be fine with getting this into master or would you still prefer to merge this PR into a separate branch and release from there?

bigopon commented 5 years ago

We should be careful with bumping another major though, 2.x seems like a good stop to sync all vCurrent versions in the future for better compatibility. Also for beta, do you get auto update to latest beta version? if not, I would say we just need to bump beta version with a reminder in the release note to warn folks.

zewa666 commented 5 years ago

@EisenbergEffect please take a look at the final discussion over at this PR. If you're fine I'd argue lets do as  @bigopon suggested, merge this into master, update the dependency to the latest i18next with this PR and do a beta release.

If you're fine please go ahead and merge this PR.

EisenbergEffect commented 5 years ago

Sounds good @zewa666

@fkleuver Is this repo switched over to the new CI process? Does this actually need to be merged to develop instead of master?

fkleuver commented 5 years ago

@EisenbergEffect It has, but the automation fails because this library is still in beta, so the automatic version bump to master doesn't work. So the updated CircleCI is still sitting in limbo on develop.

I would say stick to the old process for this one until it's no longer in beta.

EisenbergEffect commented 5 years ago

Ok, so just to confirm: I should merge this to master and release a beta version. Once we feel we're ready to move out of beta, I'll ping @fkleuver to get things sorted with the new process. Is that correct?

fkleuver commented 5 years ago

Yup

zewa666 commented 5 years ago

Sounds good @eisenbergeffect