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

Remove typescript typings #1658

Closed
levino opened this issue May 25, 2018 · 42 comments · Fixed by #1660
Closed

Remove typescript typings #1658

levino opened this issue May 25, 2018 · 42 comments · Fixed by #1660
Assignees
Labels
In Progress Currently being worked on

Comments

@levino
Copy link
Contributor

levino commented May 25, 2018

The files with types are non working and sloppy (lots of implicit any). Please remove them, they cause more harm than good.

levino added a commit to levino/web3.js that referenced this issue May 25, 2018
@levino
Copy link
Contributor Author

levino commented May 25, 2018

I created a first draft of typings for @types/web3. See DefinitelyTyped/DefinitelyTyped#26031

@eswarasai
Copy link

@levino -- I've seen that you've closed the PR at DefinitelyTyped and also raised a PR with web3.js completely removing the typings. Any particular reason why aren't we focusing on publishing @types/web3 and rather completely removing the typings from the repo? Thanks!

@levino
Copy link
Contributor Author

levino commented May 26, 2018

I just opened the PR on Friday. It is WIP, so I closed it until I am done. My idea is to have the typings only at definitelytyped and no mention of typescript in this repo over here. I am happy to get any further support on the matter. Maybe one can use the web3 repo here to gather issues for the typings over at definitelytyped. I will continue work on Monday and formulate some todos. The typings from the PR pass linting and work for me to compile my project which uses web3 1.0. please give it a spin too and see what is missing for you.

@eswarasai
Copy link

@levino -- Got it. Probably it's better to get a sign-off from the repo owners regarding the plan of action here. I've added a comment over another PR asking for their thoughts on the matter. I'd be more than happy to help you with the porting of code from web3@1.0 to DefinitelyTyped as that's the suggestible way to have typings for any library.

PS: I'm just getting started with web3@1.0 and TypeScript. Will definitely give it a spin and reach out to you in case there's something missing. Thanks for the WIP PR and also quick response. Have a great weekend :)

@levino
Copy link
Contributor Author

levino commented May 26, 2018

There is a risk that this approach won't be approved, even though it follows the categorical imperative and as such is the correct approach ;). Even if the decision is taken to keep the typings here, the PR at dt will still be very valuable. Even more so if we use it to create very well readable and high quality typings. They can easily be ported back. I also just took the typings from here and fixed the linting errors they produced in a stricter setup like dt when I created the PR in the first place. I also "stole" the typings for bn.js somewhere and have to add credit for this in the PR. One of the reasons I consider the PR WIP. I did add @simon-jentzsch as the original author already for the web3 typings.

@eswarasai
Copy link

@levino -- Understood. Let's try to get it approved though ;) Let me know how can I be of help here and also we can try asking around the folks at DefinitelyTyped maybe to give some pointers on their feedback as well along with web3.js team.

@levino
Copy link
Contributor Author

levino commented May 28, 2018

@eswarasai I added more work to it. Please review the PR at DT. DefinitelyTyped/DefinitelyTyped#26031

@eswarasai
Copy link

@levino -- Great work 👍This is looking amazing. Thanks for all the work. Hopefully we can get this merged soon :)

@levino
Copy link
Contributor Author

levino commented May 28, 2018

I added a "Todos" section in the PR. If you have time, please feel free to address any one of them. For example if would be great if you could add tests.

@levino
Copy link
Contributor Author

levino commented May 28, 2018

Mentioning @frozeman to get some feedback on the matter.

@levino
Copy link
Contributor Author

levino commented May 28, 2018

Also I kinda got lost on the way and based my work on an outdated type definitions file. I will have to fix this tomorrow (and add a bunch of contributors).

@levino
Copy link
Contributor Author

levino commented May 28, 2018

Here is another interesting issue related to this topic: #1597
This issue would have been prevented when DefinitelyTyped would have been used. Because DefinitelyTyped actually automatically tests the type definitions! The change from c0a4f0a would have thrown an error.

@levino
Copy link
Contributor Author

levino commented Jun 8, 2018

My changes to DefinitelyTyped have been merged. Now people can install and use https://www.npmjs.com/package/@types/web3. It wont work just like that at the moment as the broken type defintions still are part of the web3 package code. To fix this I leave an instruction here:

How to use @types/web3 with web3@1.0.0-beta.34

First you have to add a postinstall script in your package.json like so:

{
  "postinstall": "rm -f node_modules/web3/index.d.ts"
}

This will remove the incorrect type definitions shipped with web3 after web3 has been installed.
Then you can install web3 and @types/web3 with:

npm install web3@1.0.0-beta.34
npm install -D @types/web3

To use web3 in your ts files you have to then use

import Web3 = require('web3')

which is the correct way to import CommonJS/AMD modules in typescript.

@freeatnet
Copy link

freeatnet commented Jun 10, 2018

@levino When following these instructions on a React app boostrapped with latest @petejkim/react-scripts-ts-sass (latest npm version, 2.15.2), I get

TS1202: Import assignment cannot be used when targeting ECMAScript modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.

@levino
Copy link
Contributor Author

levino commented Jun 10, 2018

Are you sure you use the exact code above for requiring the module? Copy and paste it.

@levino
Copy link
Contributor Author

levino commented Jun 10, 2018

Can you create a minimal example repo to reproduce your issue?

@freeatnet
Copy link

@levino Here's the repro, commit messages indicating steps taken: https://github.com/freeatnet/web3-1.0.0-beta.34-types-issue-repro. I appreciate your quick response and work on web3 types!

@levino
Copy link
Contributor Author

levino commented Jun 10, 2018

I gave you two solutions to your issue in the repo via PRs. For reference the problem was usage of setting module: "esnext" in the tsconfig.json. It should (if possible) be module: "commonjs". This is more a problem of using typescript correctly with all the kinds of different modules than the typings for web3.

@freeatnet
Copy link

@levino Thank you for looking into it! I'm gonna experiment with allowSyntheticDefaultImports, as you suggested (it seems switching it on by default has been discussed at length in TypeScript issues).

Related-unrelated: if I feel that I found an issue in @types/web3, what's the best way to report it?

@levino
Copy link
Contributor Author

levino commented Jun 10, 2018

As an issue in the definitely typed repo. Highlight me if you want in the issue.

@trungfinity
Copy link

Thanks for your great work @levino!

@max-block
Copy link

@levino Can you please give an example how to use abi module with your @types/web3 package?

Now it works for me only like this (of cause without typings):
const abi = require("web3-eth-abi") // tslint:disable-line

@levino
Copy link
Contributor Author

levino commented Jun 14, 2018

My package is for web3 not for the submodules. I guess you could also use web3 directly, right? Then do that until correct types for the abi have been released.

@tomo1026j
Copy link

@levino I followed your instruction and created a simple application.
But got the following error while trying from Postman, but stuck for 2 days now.

<!DOCTYPE html>
<html>
    <head>
        <title></title>
        <link rel="stylesheet" href="/stylesheets/style.css">
    </head>
    <body>
        <h1>Cannot read property 'providers' of undefined</h1>
        <h2></h2>
        <pre>TypeError: Cannot read property 'providers' of undefined
    at router.post (/Users/tomo/Documents/GitHub/SampleWeb3/build/index.js:14:38)

Seems 'import' syntax and the typing file is okay.

My repo is https://github.com/tomo1026j/SampleWeb3

I'm sorry if this is not the right place to ask but appreciate if you can help me.

@max-block
Copy link

I've understood finally how to work with @types/web3 and abi submodule, it works fine:

import Web3 = require("web3")
const web3 = new Web3()
web3.eth.abi.encodeFunctionCall(...)

@pau1m
Copy link

pau1m commented Jul 16, 2018

@levino There's another small issue with getPastLogs where address is marked as required when it should be optional.

Have created a PR

DefinitelyTyped/DefinitelyTyped#27340

levino added a commit to levino/web3.js that referenced this issue Aug 10, 2018
levino added a commit to levino/web3.js that referenced this issue Aug 10, 2018
…escript specific things in the codebase.
@nivida nivida self-assigned this Aug 10, 2018
@nivida nivida added the In Progress Currently being worked on label Aug 10, 2018
levino added a commit to levino/web3.js that referenced this issue Aug 10, 2018
…escript specific things in the codebase.
levino added a commit to levino/web3.js that referenced this issue Aug 10, 2018
…escript specific things in the codebase.
frozeman pushed a commit that referenced this issue Aug 10, 2018
* Fix #1658 by removing the typings files and all references to typescript specific things in the codebase.

* Add documentation usage with TypeScript.
kincaidoneil added a commit to Kava-Labs/minomy that referenced this issue Aug 10, 2018
- Import it correctly as a CommonJS/AMD module in TypeScript: web3/web3.js#1658 (comment)
- Delete the default types on postinstall
dylanseago added a commit to go-faast/faast-web that referenced this issue Aug 15, 2018
@krzkaczor
Copy link

krzkaczor commented Sep 10, 2018

@levino How using import... = require can be a correct syntax if even babel doesnt support it. This would be literally the only place in code in my entire codebase using that syntax.

babel/babel#7062

@zachlysobey
Copy link

@krzkaczor that is a syntax supported by TypeScript

When exporting a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require

That said, I'm not sure that it's necessary...

@krzkaczor
Copy link

krzkaczor commented Oct 20, 2018

@zachlysobey I think its somehow deprecated, for example its not supported by babel: babel/babel#7062 This is why I really avoid it.
EDIT: okay i see that i posted the same link before xD

@levino
Copy link
Contributor Author

levino commented Oct 20, 2018

We are not talking about Babel but typescript. You can use "synthetic default exports" and it will work in all kinds of ways. But the correct way to use this package in a typescript project is as described.

Anyhow this is off topic. If you still feel some pain from this, please create a new issue with proper description.

@hevans90
Copy link

@levino thank you for your hard work on these typings & your explanations of how to get them working.

Does anyone know if there are any plans to ship these working typings as a part of the web3 package?

@levino
Copy link
Contributor Author

levino commented Oct 29, 2018

There are no plans because that would be an anti pattern. Please start reading from the top, including all links.

@D4nte
Copy link

D4nte commented Mar 12, 2019

How come there is a types/index.d.ts is now present in the repo? (version web3@1.0.0-beta.48).

I feel like it's a regression as it creates issues similar to what was mentioned in this thread.

@levino
Copy link
Contributor Author

levino commented Mar 12, 2019

Yeah. It was decided to do so. Type defs are now maintained here again

@smartcontracts
Copy link

@levino is the usage you mentioned above still correct as of the latest Web3 version?

@levino
Copy link
Contributor Author

levino commented Mar 26, 2019

No. Type definitions are now shipped with web3

@BarberDucky
Copy link

@levino Maybe I'm missing something, but are types still being shipped with web3 as of 1.2.0?
It seems like they are not. I'm importing as follows: import Web3 from 'web3', and no types.
What would be the correct way now?
Only npm i -D @types/web3 ?

@tsujp
Copy link

tsujp commented Sep 17, 2019

@levino might I be doing something wrong if types are being shipped?

Could not find a declaration file for module 'web3' <path_to_file> implicitly has an 'any' type.
Try `npm install @types/web3` if it exists or add a new declaration (.d.ts) file containing `declare module 'web3';`

14 import Web3 from 'web3'

@levino
Copy link
Contributor Author

levino commented Sep 18, 2019

Type defs are not shipped with web3@^1 at the moment. I think that @types/web3 work with web3@^1. web3@^2 is still beta, I think but it has type defs for TS.

Currently @NVIDIA does not allow us to officially provide type defintions via DT and also does not provide type defintions himself.

@tsujp
Copy link

tsujp commented Sep 18, 2019

@levino I ended up using "web3": "^2.0.0-alpha.1" and imports such as the following to get types in.

import Web3 from 'web3'
import { BlockHeader } from 'web3-eth'

Some of the APIs (I know it's alpha) are still confusing though, and always have been with web3 I think, here's an example:

function subNewBlockHeaders () {
  web3ws.eth.subscribe('newBlockHeaders', null, (err: Error, res: BlockHeader) => {
    if (err) {
      console.log('error', err)
    }
    console.log('new block', res.number)
  }).on('data', (data: BlockHeader) => {
    console.log('this will never be reached, either directly with .on or by assignment from eth.subscribe')
  })
}

// SUBSCRIBE
subNewBlockHeaders()

Yet the docs in all versions past 1 (I believe) state you have to use .on. I know this isn't really related to types but I thought I'd mention it as this is something that I didn't expect when I converted vanilla 1 code to 1.2 typed code and now 2.alpha.1 typed code.

@nivida
Copy link
Contributor

nivida commented Sep 18, 2019

@levino Could you please stop trying to restart the types discussion all the time. The 2.x branch does have the types for each module and there are way better then the types you have „maintained“ on DT. We will back-port all the bug fixes we already have done into the 1.x branch and the type definitions with the dtslint tests will get copied to the 1.x branch as well. (see issue #3070)

@web3 web3 locked as resolved and limited conversation to collaborators Sep 18, 2019
nachomazzara pushed a commit to nachomazzara/web3.js that referenced this issue Jun 4, 2020
* Fix web3#1658 by removing the typings files and all references to typescript specific things in the codebase.

* Add documentation usage with TypeScript.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
In Progress Currently being worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.