Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Bug fix: fix hdwallet types #5767

Merged
merged 2 commits into from
Dec 8, 2022
Merged

Bug fix: fix hdwallet types #5767

merged 2 commits into from
Dec 8, 2022

Conversation

eggplantzzz
Copy link
Contributor

PR description

In #3777 it was reported that hdwallet-provider could not be included in TS projects because the compiler would choke on some missing types. This turned out to be both because it was using the wrong version of @types/web3 and did not contain some necessary types packages in the regular dependencies section. The types were being exposed but were only listed under devDependencies in the package.json.

This PR pins @types/web3 in all of Truffle to 1.0.20 (which was sometimes resolved to 1.2.2 which is a stub package containing no types). It also moves a couple of dependencies into the dependencies section of package.json as those types are exposed in that package.

Testing instructions

If you would like to test this you can do the following:

  • create a new npm project (npm init -y)
  • create a tarball using npm pack in hdwallet-provider after checking out this PR's latest commit and bootstrapping
  • add an index.ts that imports hdwallet-provider
  • set up a rudimentary tsconfig.json and install TS
  • move the tarball into your new project's directory and install from it npm install -S <yourTarballFilename>
  • compile the project using typescript

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

  • I have added any needed breaking-change and new-feature labels for the appropriate packages.

@benjamincburns
Copy link
Contributor

This might be a dumb question, but why do we need @types/web3 at all now that web3 ships with types included?

@eggplantzzz
Copy link
Contributor Author

I think the types are broken...at one point they were. I'll try removing them and see if it breaks.

@eggplantzzz
Copy link
Contributor Author

Ah, also web3 doesn't contain those provider types we use that are included in @types/web3.

@benjamincburns
Copy link
Contributor

Ah, also web3 doesn't contain those provider types we use that are included in @types/web3.

If that's the only missing piece, I'd be an advocate of providing our own version of those via @truffle/provider along with a PR back to web3 that exposes good types for providers.

@eggplantzzz
Copy link
Contributor Author

Yeah I'd be into getting rid of that dep: even if it means simplifying the types.

@haltman-at
Copy link
Contributor

haltman-at commented Dec 8, 2022

This might be a dumb question, but why do we need @types/web3 at all now that web3 ships with types included?

Yeah while I don't know the current status, for quite a while at least it was the case that the types included in web3 were simply wrong and unusable, and so you still had to use @types/web3 despite web3 notionally including types. This may be fixed now, though; I haven't checked.

Edit: Here's an old issue I filed with them about this, it's now closed as fixed (and there have been many updates since then), but I don't know whether the fix was comprehensive or just fixed the stuff I pointed out.

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Makes sense to me as a quick fix, but as @benjamincburns says we should really review whether we still need @types/web3 at all. I guess I'll open an issue for that? (Edit: Put up #5772.)

@eggplantzzz eggplantzzz merged commit 868fb10 into develop Dec 8, 2022
@eggplantzzz eggplantzzz deleted the hdwallet-types branch December 8, 2022 20:12
@benjamincburns
Copy link
Contributor

Yeah all good, guys - glad you didn't interpret my comments as blocking this merge!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants