search

aurelia / i18n

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

chore: i18next version upgrade #341

Closed Sayan751 closed 3 years ago

Sayan751 commented 3 years ago

The i18next is added as a peer dependency with version restriction as >=20.3.2. This is a sort of breaking change, although it does not change any of the public API of aurelia-i18n.

This closes #335 and #340.

zewa666 commented 3 years ago

Since going forward a new min nodejs version is required, do we want to add a engines section in package.json?

Sayan751 commented 3 years ago

Since going forward a new min nodejs version is required, do we want to add a engines section in package.json?

@zewa666 Done.

zewa666 commented 3 years ago

Lgtm

josundt commented 2 years ago

@Sayan751 and @zewa666: It's been close to a year since this PR was approved and merged, but I can't see that it resulted in a new package release. i18next 14.x is now very old - current version is 21.x.

Fortunately i18next had fewer breaking changes than the version gap may suggest - actually there has been close to none that should affect the aurelia-i18n package. I just tested to force upgrading to i18next v21.x, and the only issue I had was a minor TypeScript type change (that may already have been fixed in this PR):

aurelia-i18n.d.ts:

// ....

// Before:
// import i18next from 'i18next';

// After:
import * as i18next from 'i18next';

// ...

Moving i18next to peer dependency seems to be a good move that will reduce housekeeping. Since changing to peer dependency it is a breaking change, you probably want to bump the major version in the next release..

Can you please release the new package version? We really need to get a more recent version of i18next in our products.

Sayan751 commented 2 years ago

@josundt Thank you for notifying us. I have created the PR #344. After that's merged, we will try to publish a new version.

josundt commented 2 years ago

Thanks @Sayan751. A minor suggested addition to the description for PR #344 regarding the i18next changes in 21.0.0:

Approx how long do you expect it to take until he package will be published?

Sayan751 commented 2 years ago

Singular now requires suffix _one.

Thanks @josundt for catching that. I missed that, as our tests are green even without that change.

Approx how long do you expect it to take until he package will be published?

The prep PR is already on the way: https://github.com/aurelia/i18n/pull/351.

bigopon commented 2 years ago

Thanks, 4.0.1 should be on npm.

josundt commented 2 years ago

@Sayan751 @bigopon Just tested the 4.0.1 release, and unfortunately the file dist/aurelia-i18n.d.ts is missing in the package! That means that the latest version does not work in my TypeScript project at all...

Can you please quickly release a hotfix with the typings included again?

_PS! Please read my first comment about the single required change to aurelia-i18n.d.ts compared to the 3.4.1 release. I just tested copying the .d.td file from 3.4.1 release in to node_modules/aurelia-i18n/dist and make this described change; then everything works like charm..._

bigopon commented 2 years ago

@josundt 4.0.2 is out. Thanks for notifying.

josundt commented 2 years ago

@bigopon

Now the application works perfectly with WebPack, but our Jest unit tests fail because other files referenced from package.json is missing.

While WebPack prefers the package.json "module" file reference over "main", Jest tests use "main" - and this CommonJS compiled target file that used to be there pre v4 is missing.

In my opinion there's no need for a CommonJS compiled file - UMD compiled files are interpreted exactly like a CommonJS module even in Node, and a UMD file is already included. So the only file that needs to be updated to fix this issue is package.json.

From package.json (v4.0.2) (with comments):

{
  // ...
  "main": "dist/commonjs/aurelia-i18n.js",  // ⚠️ File missing - umd works - use the same path as "unpkg" below
  "module": "dist/es2015/aurelia-i18n.js", // OK
  "browser": "dist/umd/aurelia-i18n.js", // ⚠️ File missing - use the same path as "unpkg" below
  "unpkg": "dist/umd-es2015/aurelia-i18n.js",  // OK
  "typings": "dist/aurelia-i18n.d.ts", // OK
  // ...
}

The file with my proposed changes (where the UMD target from "unpkg" above is reused for "main" and "browser"):

{
  // ...
  "main": "dist/umd-es2015/aurelia-i18n.js",
  "module": "dist/es2015/aurelia-i18n.js",
  "browser": "dist/umd-es2015/aurelia-i18n.js",
  "unpkg": "dist/umd-es2015/aurelia-i18n.js",
  "typings": "dist/aurelia-i18n.d.ts"
  // ...
}

Can you please update package.json accordingly and release one more update?

bigopon commented 2 years ago

@josundt removing a dist format without any announcement is a thing we probably wouldn't dare to do. There was some upgrade in code that requires tslib to be an explicit dependency, and the build was throwing error for amd, commonjs and native modules. I somehow missed this hence the missing dists.

Btw, if you want to target es5, better use the configuration in our Aurelia plugin:

new AureliaPlugin({
  dist: 'es2015'
})

it'll be simpler and more unified across all core packages.

And 4.0.3 should be out on npm with all the dist.

josundt commented 2 years ago

@bigopon In 4.0.3 everything's all good 👍

Nice that all the compiled targets are back. My suggestion was meant as a "least effort" solution to solve the issues with 4.0.2.

And thanks for the tip about the dist option for AureliaPlugin! I've read the plugin documentation before but never picked up what this property was for. We have not set this property until now, and therefore have bundled the es5 versions of the aurelia-* packages. Since our own products now target es2019, I will set the dist property to es2017.