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

chore(typescript): bump tsc globally and switch to nodenext for iroha2 #2179

Merged

Conversation

aldousalvarez
Copy link
Contributor

@aldousalvarez aldousalvarez commented Oct 25, 2022

Here are all the changes implemented for
moduleResolution to nodeNext and Typescript upgrade

  1. Upgraded TypeScript to 4.7.4 globally and switched
    moduleResolution nodeNext in cactus-plugin-ledger-connector-iroha2
  2. Upgraded @typescript-eslint/eslint-plugin &
    @typescript-eslint/eslint/parser to 5.27.0
    so that it supports the newer version of Typescript.
  3. Added useUnknownInCatchVariables: false in tsconfig
    and tsconfig.base.json to fix the errors related to
    'unknown' type in catch variables that occurred after
    upgrading the typescript.
  4. Updated rxjs to v7.8.1 globally in all package.json files
    to match the version in IrohaV2. This is better than adding a resolution
    in the root package.json because it makes it visible and explicit that
    we've upgraded the dependency everywhere. The global resolutions
    override is a last resort option because it is pretty confusing to
    beginners when they specify a dependency version in their own package.json
    but it doesn't ever get installed because of the override coming from the
    top level package.json.
    The reason the upgrade was needed to begin with: Typescript compiler
    errors were being caused by the mismatching version of rxjs that the
    iroha2 client packages depend on vs. the versions that our codebase
    uses. Now with the upgrade they are in sync and the build passes.

Secondary change:
Peter: I'm sneaking in another hot-fix for the codegen task which makes
the .jar download (that's part of the warmup script) SEQUENTIAL instead
of having it PARALLEL which appeared to be working fine earlier but it
no longer seems to be the case, e.g., I've seen some race condition
looking errors pop up after having merged the PR with the earlier hotfix.
The root cause of the issue likely the openapi-generator-cli package
itself not being safe for concurrent use in general, but a full audit of
that code would have to be performed to validate this theory. For now
it is just easier to set it to SEQUENTIAL and be done with it (hopefully)

Fixes #2158

Co-authored-by: Peter Somogyvari [email protected]

Signed-off-by: aldousalvarez [email protected]
Signed-off-by: Peter Somogyvari [email protected]

@gitguardian
Copy link

gitguardian bot commented Oct 25, 2022

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2158 branch 2 times, most recently from 7f6493e to 301e34a Compare November 17, 2022 05:01
@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2158 branch 2 times, most recently from e5266bb to 0c7c9c8 Compare November 21, 2022 06:58
@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2158 branch from 0c7c9c8 to 8c99118 Compare February 16, 2023 07:54
@aldousalvarez aldousalvarez marked this pull request as ready for review February 17, 2023 04:03
Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just fix the PR commit message.
You can refer to this: 57eb605 (#2207)

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2158 branch from 8c99118 to 92e85bd Compare February 24, 2023 10:57
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aldousalvarez This is a great work, but I'd strongly prefer breaking it down to multiple smaller pull requests which are all focused on changing a single thing (for example one pull request that does nothing but upgrading to Typescript 4.7.x)

But, before we even get to that, here's another question for me to get more context: It looks to me like we could do the change to moduleResolution on a per-package basis (once the compiler is up on >=v4.7.x) therefore we could limit the scope of this PR to a much smaller scale that does nothing but exactly satisfying the requirement from @outSH which is that in a specific package he'd like to use a dependency that requires the new module resolution mechanism.

So, the question: Could we break this whole work down in this way:

  1. The compiler upgrade to >v4.7
  2. PR that switches only the specific iroha connector package to moduleResolution nodenext
  3. N number of PRs later on that can change more packages to the new module resolution over time - thus breaking this giant diff down to smaller, digestible chunks?

Also note (to myself mostly) that we want the flag turned on in the future that forces developers to deal with the runtime type of the thrown exceptions (which is something that Typescript 4.4 introduced as a feature that we - me at least - very much want enabled in our codebase)

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2158 branch from 92e85bd to 5f91646 Compare June 7, 2023 08:11
@aldousalvarez
Copy link
Contributor Author

Hello @petermetz here is the updated commit with moduleResolution to nodenext and typescript upgrade to 4.7.4 is only on iroha2 connector

@aldousalvarez aldousalvarez requested a review from petermetz June 15, 2023 00:50
@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2158 branch 3 times, most recently from 0f0cf91 to 1ae2456 Compare June 15, 2023 07:09
@petermetz
Copy link
Contributor

Hello @petermetz here is the updated commit with moduleResolution to nodenext and typescript upgrade to 4.7.4 is only on iroha2 connector

@aldousalvarez You upgrade the compiler to v4.7 project wide not just for the package but that's okay because this is the only way to do it anyway. I meant to send that in a separate PR, but the diff is not that large now that you cut down on the rest of the packages so I'm just gonna let it go because the CI is way too slow right now so I'm trying to be a little less nit-picky about mixing different commits into a single PR.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aldousalvarez Please explain in the commit message the reason behind each change.
Your point 4 explains why it was done but 1,2,3 just says "this is what I did" but not "why".

If someone comes back to this later because we broke something else that we didn't realize at the time, they have to be able to figure out why we made the changes we made so that they can make better/more educated decisions about what the fix might be (and ideally they need to be able to do that without having to have a discussion about it with you or me)

Other than that, the PR diff itself is LGTM, thank you!

@petermetz petermetz requested a review from outSH June 16, 2023 23:26
@aldousalvarez
Copy link
Contributor Author

Hello @petermetz already updated the commit message and I have explained there why I did those changes. Thank you

Here are all the changes implemented for
moduleResolution to nodeNext and Typescript upgrade
-------------------------------------------
1. Upgraded TypeScript to 4.7.4 globally and switched
moduleResolution nodeNext in cactus-plugin-ledger-connector-iroha2
2. Upgraded @typescript-eslint/eslint-plugin &
@typescript-eslint/eslint/parser to 5.27.0
so that it supports the newer version of Typescript.
3. Added useUnknownInCatchVariables: false in tsconfig
and tsconfig.base.json to fix the errors related to
'unknown' type in catch variables that occurred after
upgrading the typescript.
4. Updated rxjs to v7.8.1 globally in all package.json files
to match the version in IrohaV2. This is better than adding a resolution
in the root package.json because it makes it visible and explicit that
we've upgraded the dependency everywhere. The global resolutions
override is a last resort option because it is pretty confusing to
beginners when they specify a dependency version in their own package.json
but it doesn't ever get installed because of the override coming from the
top level package.json.
The reason the upgrade was needed to begin with: Typescript compiler
errors were being caused by the mismatching version of rxjs that the
iroha2 client packages depend on vs. the versions that our codebase
uses. Now with the upgrade they are in sync and the build passes.

---
Secondary change:
Peter: I'm sneaking in another hot-fix for the codegen task which makes
the .jar download (that's part of the warmup script) SEQUENTIAL instead
of having it PARALLEL which appeared to be working fine earlier but it
no longer seems to be the case, e.g., I've seen some race condition
looking errors pop up after having merged the PR with the earlier hotfix.
The root cause of the issue likely the openapi-generator-cli package
itself not being safe for concurrent use in general, but a full audit of
that code would have to be performed to validate this theory. For now
it is just easier to set it to SEQUENTIAL and be done with it (hopefully)

Fixes hyperledger-cacti#2158

Co-authored-by: Peter Somogyvari <[email protected]>

Signed-off-by: aldousalvarez <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz force-pushed the aldousalvarez/issue2158 branch from 98f884f to 589c83f Compare June 21, 2023 21:56
@petermetz petermetz changed the title chore(typescript): upgrade and switch to nodenext chore(typescript): bump tsc globally and switch to nodenext for iroha2 Jun 21, 2023
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aldousalvarez LGTM now, but I've made some changes:

  1. Instead of a global resolution I've upgrade rxjs explicitly via the dependency declarations. Same end result but less confusion. I further explained this in the commit message. The diff is much larger this way but much easier to understand/reason about it and so it's less technical debt (IMO).
  2. I snuck in yet another hot-fix for the codegen script which is still suffering from concurrency issues.

@jagpreetsinghsasan @takeutak @izuru0 @VRamakrishna @sandeepnRES Please re-review with the above change in mind.

@petermetz petermetz enabled auto-merge (rebase) June 21, 2023 22:00
Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@petermetz petermetz merged commit 6742817 into hyperledger-cacti:main Jun 22, 2023
@petermetz petermetz deleted the aldousalvarez/issue2158 branch June 22, 2023 20:17
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.

chore(typescript): upgrade and switch to nodenext
5 participants