transifex / transifex-javascript

Transifex Native SDK for Javascript
Apache License 2.0
42 stars 14 forks source link

chore: Upgrade to Angular v14 #186

Closed rgant closed 1 year ago

rgant commented 1 year ago

DESCRIPTION: Angular v13 support window ends 2023-05-04. Angular v16 is expected 2023-05-01 (BTW documentation does not express support for Angular v15 currently. But the code does. Should this be updated?) My analysis of the current Angular code shows nothing that is a breaking change from Angular v13 to v15.

Additionally I have refreshed the dependencies and updated the lint configuration to current recommendations from Angular / @angular-eslint/eslint-plugin. All code changes are automatic formatting and fixing from tooling.

Finally I have run npx package-json-validator --warnings --recommendations and npx sort-package-json.

STEPS TO TEST:

  1. ng lint
  2. ng test
  3. ng build

All linting and tests pass on my system. Build appears to be good as well. All of my future contributions to this library will likely be based off of these changes.

rgant commented 1 year ago

Merging this branch will close #187

pablotransifex commented 1 year ago

@rgant I appreciate the effort you put in linting and cleaning the package. We in the beginning decided to go with the Airbnb linting standards in general for the TXNative repository, but I actually was aware of the specific linting rules and standards in Angular development. I'll check these changes and let you know how we decided to proceed.

Again, thank you very much.

pablotransifex commented 1 year ago

@rgant It seems that the tests have some problem, in order to reproduce locally, please run in the root folder of the repo:

and in the angular folder:

We are using Lerna to build.

rgant commented 1 year ago

I'll look into the error, but best I can tell that is a build error and has less to do with my code and more to do with old versions of lerna and old versions of Angular (14 is still old) with brand new versions of NodeJS v18.

I tend to expect that Transifex isn't going to wholly accept my changes. Which is fine, but I did promise to suggest improvements before based on my company's experience using your tools. We have completely abandoned this Angular implementation because of problems with how it works. I will be completely re-writing everything here to work in (what I believe is) a more Angular-y way.

It's up to you if you want to wait to see the final results before reviewing, or review individually. I won't open any other pull requests here, but I will be recording them in my fork for you to look at if you desire.

pablotransifex commented 1 year ago

@rgant It seems so, a build problem, indeed, but I don't know how to solve it yet. I'll look into it too.

I'll be checking your changes in your forked repository a well, thank you!

rgant commented 1 year ago

I believe the issue is that the library's package.json includes Angular 12 and 13. But I haven't had a chance yet to try and remove those.

pablotransifex commented 1 year ago

@rgant The problem was this part in the package.json inside the library:

  "dependencies": {
    "tslib": "^2.3.0"
  }

this generates a node_modules inside it that is not the same as in the root folder, and thus the discrepancy.

I pushed a commit, let's see if now it's ok (locally the tests ran ok).

rgant commented 1 year ago

Good catch, I will add documentation about removing that since that is the default when Angular v14 creates a library.

pablotransifex commented 1 year ago

@rgant I have errors when compiling in Angular 12 and 13, like this one: image

using this PR is breaking those versions, is this expected?

rgant commented 1 year ago

Angular 12 and 13 are no longer supported versions of Angular: https://angular.io/guide/releases#actively-supported-versions

pablotransifex commented 1 year ago

@rgant correct, I was thinking in creating a new release with the v14+ for angular and keep another release for upto v13. This way we can support older versions with the current implementation, and improve the new installations with your refactoring (when ready).

From my side there is no problem, I would like to discuss this and back to you.

pablotransifex commented 1 year ago

@rgant correct, I was thinking in creating a new release with the v14+ for angular and keep another release for upto v13. This way we can support older versions with the current implementation, and improve the new installations with your refactoring (when ready).

What do you think?

rgant commented 1 year ago

That is one way. I think a more typical way is to say that users of old versions of frameworks should install old versions of add-ons.

egilkh commented 1 year ago

I agree with rgrant. And hope this can be merged :)

pablotransifex commented 1 year ago

I agree with rgrant. And hope this can be merged :)

@egilkh Due to vacations we cannot do it now, but we will as soon as possible.

pablotransifex commented 1 year ago

@rgant Can you please rebase and resolve conflict in this PR in order to move on and release?

pablotransifex commented 1 year ago

@rgant I don't know what changed since last time, but there are errors in the compilation, related to angular dependencies, also notices that you added node and npm versions to package.json, that should not be there.

Last time we build this PR was good, I don't know what changed and now is breaking.

pablotransifex commented 1 year ago

@rgant Closing this, we have fixed the build on another PR already merged. Feel free to let us know any other improvements that we can apply for the Angular 14 version we will release soon. And thank you very much again for the effort.