truffle
truffle copied to clipboard
trufflesuite
Standardize `@types/node` version and upgrade all TS packages to use `[email protected]`
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, 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.
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).
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.
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/nodeupon 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.
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.
@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).
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.
:thinking: 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?
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.
:thinking: 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 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.
@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.