Standardize `@types/node` version and upgrade all TS packages to use `[email protected]` #5423

benjamincburns · 2022-08-10T13:10:46Z

So the main goal here was to update to latest TypeScript, as that has support for the ES2022 lib, which is required to support error chaining (#5367).

In the process of performing this upgrade, I noticed that even though we have a very well-known minimum supported version of NodeJS, five of the six different versions of @types/node upon which we depend were targeted at the wrong node version, and included API features that aren't present in our minimum supported node version.

[Edit: corrected above paragraph]

These changes shipped in two different commits, with each commit being the complete set of changes necessary to deliver the stated goal. That is, each commit contains the obvious change to support the primary motive, and then whatever follow-on changes were necessary to ensure that I wasn't going to break CI.

Please see the commit messages for those two commits for the rationale behind each of the follow-on changes.

As for why this isn't two separate commits, it's primarily because testing this was a lot more effort than I initially expected, and I didn't want to have to potentially triple that effort by having to test these as both independent and integrated changes (which is what would be required for two separate PRs).

benjamincburns · 2022-08-10T13:21:16Z

Also ICYMI in the dense commit messages, re: the new typedoc warnings on build of docs for db and db-kit, I've disabled those warnings successfully now and raised #5421 and #5420 to cover the effort required to re-enable the warnings.

cds-amal · 2022-08-10T14:19:49Z

In the process of performing this upgrade, I noticed that even though we have a very well-known minimum supported version of NodeJS, exactly zero of the six different versions of @types/node upon which we depend were targeted at this version, and all of the versions that we were using included API features that aren't present in our minimum supported node version.

Yikes! Thanks so much for catching this!

These changes shipped in two different commits, with each commit being the complete set of changes necessary to deliver the stated goal. That is, each commit contains the obvious change to support the primary motive, and then whatever follow-on changes were necessary to ensure that I wasn't going to break CI.

Please see the commit messages for those two commits for the rationale behind each of the follow-on changes.

This makes sense. FWIW, It's easier to review the distinct, and "complete", commits.

As for why this isn't two separate commits, it's primarily because testing this was a lot more effort than I initially expected, and I didn't want to have to potentially triple that effort by having to test these as both independent and integrated changes (which is what would be required for two separate PRs).

Strongly agree here.

cds-amal

This looks good to me! I left one nit and noticed polyfill logic was duplicated.

cds-amal · 2022-08-10T14:29:01Z

packages/compile-solidity/src/polyfill.ts

@@ -0,0 +1,11 @@
+// must polyfill AbortController to use axios >=0.20.0, <=0.27.2 on node <= v14.x


Minor nit: I think the filename should reflect the type of polyfill, like abort-controller-pollyfill.ts or something similar, instead of polyfill.ts

I guess we shouldn't worry about this If the intention is to revisit and have a polyfill module as followup work.

cds-amal · 2022-08-10T14:31:08Z

packages/dashboard-message-bus-client/lib/polyfill.ts

+// must polyfill AbortController to use axios >=0.20.0, <=0.27.2 on node <= v14.x
+import { AbortController, AbortSignal } from "node-abort-controller";


Second use; duplicate, grin and bear it :)

cds-amal · 2022-08-10T14:33:43Z

packages/source-fetcher/lib/polyfill.ts

+// must polyfill AbortController to use axios >=0.20.0, <=0.27.2 on node <= v14.x
+import { AbortController, AbortSignal } from "node-abort-controller";


3rd time, perhaps consider factoring this out to a utility module? Maybe this warrants a new truffle-package to handle these utility functions? Would be interested to hear what others think.

So I don't know what is better here; 6 lines of code duplicated 4 times (30 times) or 30 separate packages which each contain 6 lines of code (and the necessary, standard files for an npm package) that now need to be maintained for instances like this. We don't really have many shared utility-type packages.

Generally people seem opposed to ideas like this but one possibility would be to have a @truffle/common or something? I mean, this is such a small piece of code though. I'm not sure if it is worth worry over.

I'd be opposed to a @truffle/common on the basis of the naming not really defining a well-defined scope (these are sometimes referred to as "god modules").

That said, I think a @truffle/polyfills that behaves like the module here (applies polyfills as a side-effect of module evaluation) is probably warranted.

This PR is currently blocking #5367. My request is that you guys allow this bit of cruft through on this PR, and then let me clean it up in #5367, as that PR will introduce a second polyfill, and personally I find it easier to structure modules like the one I'm proposing here (@truffle/polyfills) when I have more than a single concern to place on them.

If you guys agree, I'll add a note to #5367 with a link back to this convo so that it won't be missed on review of that one.

That sounds like a good idea @benjamincburns.

Note added. #5367 (comment)

packages/db/typedoc.js

eggplantzzz · 2022-08-10T14:48:50Z

packages/hdwallet-provider/test/provider.test.ts

@@ -65,7 +65,7 @@ describe("HD Wallet Provider", function () {
          "http://localhost:8545"
        );
        assert.fail("Should throw on invalid mnemonic");
-      } catch (e) {
+      } catch (e: any) {


Do these even need to be typed? They all should be of type Error, no? And the same with the other instances of this in this PR.

IIRC, typed catch handlers are now required when noImplicitAny is true (as it is in all of our tsconfig.json) files.

Also FWIW, I've verified that if any code changes here were reverted, the build would break.

Ok that makes sense. But even so, they should be of type Error right? I mean maybe it doesn't matter and it's all just pedantic.

No, there is no requirement to throw an object of type Error in JavaScript. You can throw whatever you like. This is why the TS compiler only allows a type of any here.

I stand corrected, it can also be type unknown.

Apparently best practice is to use unknown with type guards when you find yourself wishing that you had Java or C# like typed error handlers, but given that this is existing code that isn't throwing due to catching a type with a missing message property, I think that'd be complete overkill here.

benjamincburns · 2022-08-10T15:06:41Z

Polyfill logic was duplicated to ensure that it would load properly under each of the different command bundles.

An alternative would have been to make webpack handle injecting the polyfill, but then it wouldn't be present on the unpacked build.

benjamincburns · 2022-08-10T15:14:27Z

@cds-amal I could see making a shared polyfill module, but I think that would be a better fit for another PR. I suspect there are other polyfills that we'd want to pull into said module (e.g. the error chaining one that's currently in draft status will need to polyfill the Error type to chain properly on node@<18).

cds-amal · 2022-08-10T15:15:24Z

Polyfill logic was duplicated to ensure that it would load properly under each of the different command bundles.

An alternative would have been to make webpack handle injecting the polyfill, but then it wouldn't be present on the unpacked build.

🤔 The only clients affected would be us. It could impact our ability to debug scenarios under Node 12. I don't think anyone else bootstraps to run truffle, so maybe there's merit in a webpack approach. @eggplantzzz, @haltman-at, @gnidan, @sukanyaparashar, @lsqproduction, @cliffoo thoughts?

benjamincburns · 2022-08-10T15:27:53Z

Polyfill logic was duplicated to ensure that it would load properly under each of the different command bundles.

An alternative would have been to make webpack handle injecting the polyfill, but then it wouldn't be present on the unpacked build.

🤔 The only clients affected would be us. It could impact our ability to debug scenarios under Node 12. I don't think anyone else bootstraps to run truffle, so maybe there's merit in a webpack approach. @eggplantzzz, @haltman-at, @gnidan, @sukanyaparashar, @lsqproduction, @cliffoo thoughts?

@cds-amal I have a fairly strong preference against using webpack to load the polyfills, simply because I think it's dangerous precedent. It would be fine for this change, but the next one that will be added will be for a much more modern feature (error chaining, introduced in ES2022). With a more modern polyfill we are far more likely to see differences in behavior between the packed and unpacked node versions.

cds-amal · 2022-08-11T04:03:24Z

@cds-amal I have a fairly strong preference against using webpack to load the polyfills, simply because I think it's dangerous precedent. It would be fine for this change, but the next one that will be added will be for a much more modern feature (error chaining, introduced in ES2022). With a more modern polyfill we are far more likely to see differences in behavior between the packed and unpacked node versions.

I agree. My preference is a polyfill module, as we'll have to massage the environment when we drop/add the next set of node versions.

cds-amal

Thanks, @benjamincburns! I'm approving, though I think @eggplantzzz may have something to add.

benjamincburns · 2022-08-11T05:04:39Z

@eggplantzzz I'm going to go ahead and merge this (assuming the build is green once I rebase on develop), as I think the only dangling thread was the question about the catch type, and I doubt that you want to block this on the typed error handler pattern that I mentioned there. That said, if you had other concerns please raise them to me and I'll be happy to create a new PR to address them, with priority.

This commit standardizes all occurrences of the `@types/node` to version range `~12.12.0`. This version range gives us the latest patch release of the node types that apply to the earliest LTS release of node v12 (v12.13.0). This change uncovered two node version compatibility issues that had been undetected prior to this change: 1. The version of `axios` that we use (v0.27.2) depends on the `AbortController` object that was introduced in node v15.0.0. 2. The version of the `conf` package that was used by @truffle/config was required a bugfix to build properly with earlier versions of `@node/types`. As a result, this change includes the following fixes: 1. Includes the `node-abort-controller` polyfill and adds a bit of logic to conditionally install it on node versions where the `AbortController` is not yet present. 2. Bumps the version of the `conf` package to `10.1.2`, which includes a types fix that makes the package compatible with the earliest LTS v12 release.

In order to avoid committing a broken build, the upgrade above required the following additional changes: - Upgrades @truffle/db and @truffle/db-kit devDependency `[email protected]` to `[email protected]`. - This was necessary because `[email protected]` does not support `[email protected]`. - This is not the latest release of `typedoc`, however it is the latest available release that is compatible with _both_ TypeScript v4.7 and node v12. - Support for `[email protected]` was dropped in `[email protected]`. - Replaces devDependency `@zerollup/ts-transform-paths` with `[email protected]` in `@truffle/codec` - This was necessary because `@zerorollup/ts-transform-paths` is not compatible with `[email protected]` - Removes devDependency `typedoc-neo-theme` from `@truffle/db`. - This was necessary because `typedoc-neo-theme` is tightly coupled to specific versions of `typedoc`, and no release of this theme is available that is compatible with `[email protected]`. - Thankfully this theme appears to have been inactive in the typedoc config already. - Disables `typedoc` warnings about documented types that reference other undocumented types - This change keeps the set of warnings as they were prior to this commit. - Warning on undocumented types was introduced as of `typedoc` v0.22.12.

benjamincburns requested review from gnidan, cds-amal and haltman-at August 10, 2022 13:11

This was referenced Aug 10, 2022

Typedoc missing on numerous types in @truffle/db-kit #5422

Open

Typedoc missing on numerous types in @truffle/db #5421

Open

cds-amal reviewed Aug 10, 2022

View reviewed changes

eggplantzzz reviewed Aug 10, 2022

View reviewed changes

cds-amal self-requested a review August 11, 2022 04:03

cds-amal approved these changes Aug 11, 2022

View reviewed changes

benjamincburns mentioned this pull request Aug 11, 2022

Error chaining #5367

Closed

benjamincburns added 2 commits August 11, 2022 17:05

benjamincburns force-pushed the typescript-4.7 branch from 39eda2d to 9c8eca9 Compare August 11, 2022 05:05

benjamincburns merged commit fd78ee6 into develop Aug 11, 2022

benjamincburns deleted the typescript-4.7 branch August 11, 2022 06:00

benjamincburns mentioned this pull request Aug 17, 2022

Internal improvement: Fully convert compile-solidity to use TypeScript #5432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize `@types/node` version and upgrade all TS packages to use `[email protected]` #5423

Standardize `@types/node` version and upgrade all TS packages to use `[email protected]` #5423

benjamincburns commented Aug 10, 2022 •

edited

Loading

benjamincburns commented Aug 10, 2022 •

edited

Loading

cds-amal commented Aug 10, 2022

cds-amal left a comment

cds-amal Aug 10, 2022

cds-amal Aug 11, 2022

cds-amal Aug 10, 2022

cds-amal Aug 10, 2022

eggplantzzz Aug 10, 2022 •

edited

Loading

benjamincburns Aug 11, 2022

cds-amal Aug 11, 2022

benjamincburns Aug 11, 2022

eggplantzzz Aug 10, 2022 •

edited

Loading

benjamincburns Aug 10, 2022

benjamincburns Aug 10, 2022

eggplantzzz Aug 10, 2022 •

edited

Loading

benjamincburns Aug 10, 2022

benjamincburns Aug 11, 2022

benjamincburns Aug 11, 2022

benjamincburns commented Aug 10, 2022

benjamincburns commented Aug 10, 2022 •

edited

Loading

cds-amal commented Aug 10, 2022 •

edited

Loading

benjamincburns commented Aug 10, 2022 •

edited

Loading

cds-amal commented Aug 11, 2022

cds-amal left a comment

benjamincburns commented Aug 11, 2022 •

edited

Loading

		@@ -0,0 +1,11 @@
		// must polyfill AbortController to use axios >=0.20.0, <=0.27.2 on node <= v14.x

		// must polyfill AbortController to use axios >=0.20.0, <=0.27.2 on node <= v14.x
		import { AbortController, AbortSignal } from "node-abort-controller";

Standardize @types/node version and upgrade all TS packages to use [email protected] #5423

Standardize @types/node version and upgrade all TS packages to use [email protected] #5423

Conversation

benjamincburns commented Aug 10, 2022 • edited Loading

benjamincburns commented Aug 10, 2022 • edited Loading

cds-amal commented Aug 10, 2022

cds-amal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggplantzzz Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincburns commented Aug 10, 2022

benjamincburns commented Aug 10, 2022 • edited Loading

cds-amal commented Aug 10, 2022 • edited Loading

benjamincburns commented Aug 10, 2022 • edited Loading

cds-amal commented Aug 11, 2022

cds-amal left a comment

Choose a reason for hiding this comment

benjamincburns commented Aug 11, 2022 • edited Loading

Standardize `@types/node` version and upgrade all TS packages to use `[email protected]` #5423

Standardize `@types/node` version and upgrade all TS packages to use `[email protected]` #5423

benjamincburns commented Aug 10, 2022 •

edited

Loading

benjamincburns commented Aug 10, 2022 •

edited

Loading

eggplantzzz Aug 10, 2022 •

edited

Loading

eggplantzzz Aug 10, 2022 •

edited

Loading

eggplantzzz Aug 10, 2022 •

edited

Loading

benjamincburns commented Aug 10, 2022 •

edited

Loading

cds-amal commented Aug 10, 2022 •

edited

Loading

benjamincburns commented Aug 10, 2022 •

edited

Loading

benjamincburns commented Aug 11, 2022 •

edited

Loading