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

Dependency update: Bump web3 to version 1.2.2 #2488

Closed
wants to merge 3 commits into from
Closed

Conversation

eggplantzzz
Copy link
Contributor

No description provided.

@gjgd
Copy link

gjgd commented Oct 27, 2019

Hi @eggplantzzz anything I can do to help merging this? It is blocking a fix that we want to merge in https://github.com/decentralized-identity/element/

@cgewecke
Copy link
Contributor

cgewecke commented Oct 27, 2019

@eggplantzzz If you're seeing tsc errors triggered by web3, this config might be helpful as a stopgap (see thread in web3 3134)

{
    "compilerOptions": {
        "skipLibCheck": true
    }
}

The types are 'stapled on' there and it will likely take some time to get everything straightened out.

@gnidan
Copy link
Contributor

gnidan commented Oct 28, 2019

Oh yikes, I don't think that skipLibCheck is a good idea, unless there's a way to skip only web3. I don't think we should skip typechecking all declaration files all across Truffle!

@cgewecke
Copy link
Contributor

cgewecke commented Oct 28, 2019

@gnidan The setting suppresses compilation errors from node_modules deps. Is this a big problem?

When I google it, the second entry I see (after the official TS page) is titled Why is skipLibCheck not the default setting?. It seems like for many libs the exported types don't have an organic relationship to the underlying code and are often 'off'. And there's an endless discussion going on about type correctness in deps that's not critical to the thing people are directly working on.

@cgewecke
Copy link
Contributor

For further reference, a couple other Ethereum monorepo projects using this in their tsconfig

@eggplantzzz
Copy link
Contributor Author

@gjgd We just need to get this to pass the CI. Currently it looks like there are some typing issues, I'm currently unsure of how to proceed. I suppose I need to do some research.

@eggplantzzz
Copy link
Contributor Author

eggplantzzz commented Oct 28, 2019

@cgewecke I'm not sure I understand exactly what the problem is, but won't that option turn off a huge piece of the functionality of TS? I mean, isn't type checking one of the big reasons to use TS?

@cgewecke
Copy link
Contributor

cgewecke commented Oct 28, 2019

@eggplantzzz They're looking for a reproduction case as we speak in web3 3154 because the proximate cause isn't obvious. Are you just seeing this on the HDWallet? Have you run it locally?

AFAIK the setting does not disable type-hints, but rather smooths out compilation setting discreps. There's a pretty good discussion here about how selective it is.

@cgewecke
Copy link
Contributor

@eggplantzzz Live update: web3 is diagnosing this as problem in the host config (e.g at Truffle) and related to your use of typeroots.

Does tsc work without those paths specified. Is there a design decision there you could clarify? Not a typescript expert and interested in learning more :)

@gnidan
Copy link
Contributor

gnidan commented Oct 28, 2019

AFAIK the manual paths are there as a stopgap. Happy to remove if it makes this work!

@eggplantzzz
Copy link
Contributor Author

PR for adjusting web3 typings.
web3/web3.js#3177

@eggplantzzz
Copy link
Contributor Author

Closing this in favor of #2558

@eggplantzzz eggplantzzz closed this Nov 7, 2019
@eggplantzzz eggplantzzz deleted the update-web3 branch November 21, 2019 11:34
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.

4 participants