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

Fix Web3 provider typings #3824

Closed
wants to merge 1 commit into from
Closed

Fix Web3 provider typings #3824

wants to merge 1 commit into from

Conversation

daffl
Copy link
Contributor

@daffl daffl commented Dec 23, 2020

Description

This pull request fixes the typings of the web3-provider-* packages. The modules use the default (module.exports =) export instead of a named export. The typing tests were only passing because the configuration aliased to the deprecated web3-providers module. This means the provider modules could not be used as intended in TypeScript because the named export does not exist and caused a runtime error and the default export was not typed.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build and tested dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.034% when pulling 134b34c on bidalihq:provider-types into 27c9679 on ethereum:1.x.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@spacesailor24
Copy link
Contributor

Hi there! Thank you for opening the PR!

I'm not incredibly familiar with Typescript, so forgive my ignorance, but it seems to be that named exports are more preferable and should work all the same?

@spacesailor24 spacesailor24 added the Question General discussion questions label Jan 7, 2021
@daffl
Copy link
Contributor Author

daffl commented Jan 7, 2021

They are but the corresponding JavaScript code is assigning module.exports = which is a default export, see e.g. https://github.com/ChainSafe/web3.js/blob/134b34c568f0585497746861a7ac7562b52d189e/packages/web3-providers-http/src/index.js#L142

Changing those to a named export would be a breaking change.

@spacesailor24 spacesailor24 removed the Question General discussion questions label Jan 11, 2021
@spacesailor24
Copy link
Contributor

@daffl Can you update this with your latest changes? This lgtm

@GregTheGreek Do we need to remove the CLA bot from this?

@GregTheGreek
Copy link
Contributor

Cla is disabled so we should be able to merge without it

@daffl
Copy link
Contributor Author

daffl commented Jan 11, 2021

Rebased with latest, I don't think any other changes were necessary.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 478477210

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 385 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-2.5%) to 73.962%

Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 88.0%
packages/web3-core-helpers/src/formatters.js 24 79.65%
packages/web3-core-helpers/src/errors.js 31 4.41%
packages/web3-utils/src/soliditySha3.js 55 5.56%
packages/web3-utils/src/index.js 60 30.0%
packages/web3-utils/src/utils.js 84 13.24%
packages/web3-eth-accounts/src/index.js 130 31.65%
Totals Coverage Status
Change from base Build 467243265: -2.5%
Covered Lines: 3286
Relevant Lines: 4211

💛 - Coveralls

@spacesailor24
Copy link
Contributor

If you could just update the CHANGELOG.md, this would be good to go

@koraykoska
Copy link

Correct me if I am wrong but if you change it to default exports now the changes are breaking for people using TS already right? I am not sure if we can do this in a minor release.

@daffl
Copy link
Contributor Author

daffl commented Feb 10, 2021

Right now you can't use these modules directly with TS at all without ignoring the typings:

import WebsocketProvider from 'web3-providers-ws';

new WebsocketProvider(connect, options)

Gives you a compile time error like

This expression is not constructable.
  Type 'typeof import("web3-providers-ws/types/index")' has no construct signatures.

And

import { WebsocketProvider } from 'web3-providers-ws';

new WebsocketProvider(connect, options)

Throws a runtime error with

web3_providers_ws_1.WebsocketProvider is not a constructor

@spacesailor24 spacesailor24 added P2 Medium severity bugs Types Incorrect or missing types labels Mar 9, 2021
@spb-web
Copy link

spb-web commented Mar 13, 2021

Temporary solution
const Web3Eth = require('web3-eth') as typeof import('web3-eth')['Eth']

@github-actions
Copy link

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label May 13, 2021
@github-actions github-actions bot closed this May 27, 2021
@daffl daffl deleted the provider-types branch May 27, 2021 00:19
@mooo-ooo
Copy link

hi guys, is this solved ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Medium severity bugs Stale Has not received enough activity Types Incorrect or missing types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants