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

feat: expand tsconfig with useful settings #23

Merged
merged 2 commits into from
May 19, 2020
Merged

Conversation

dryajov
Copy link
Contributor

@dryajov dryajov commented Jun 24, 2019

Settings suggested in ethereumjs-devp2p Port to typescript PR.

@dryajov
Copy link
Contributor Author

dryajov commented Jun 24, 2019

Hmm... LGTM fails with a [2019-06-24 19:24:13] [build] No JavaScript or TypeScript code found. error. Has this repo been previously ran with LGTM?

Any help here is appreciated as I'm not very familiar with this setup. @holgerd77

@alcuadrado alcuadrado self-requested a review June 28, 2019 19:35
Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

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

We already discussed this change in the devp2p TS migration PR.

LGTM should probably be disabled for this repo.

@holgerd77
Copy link
Member

@alcuadrado Currently LGTM is set to be applied to just all repos I wouldn't want to change to manual repo selection since this would mean significant extra configuration work. I we stumble upon this more often we can nevertheless change mid-term.

@holgerd77
Copy link
Member

holgerd77 commented Jun 29, 2019

My feeling for TypeScript behavior is still limited and I still have some difficulties grasping the implications of these changes.

When I add e.g. the new esModuleInterop to true setting to the current VM config, I get the following errors on compilation:

> ethereumjs-config-build

+ exec tsc -p ./tsconfig.prod.json
lib/bloom/index.ts:16:7 - error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'typeof internal' has no compatible call signatures.

16       assert(bitvector.length === BYTE_SIZE, 'bitvectors must be 2048 bits long')
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  lib/bloom/index.ts:1:1
    1 import * as assert from 'assert'
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

lib/bloom/index.ts:26:5 - error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'typeof internal' has no compatible call signatures.

26     assert(Buffer.isBuffer(e), 'Element should be buffer')
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  lib/bloom/index.ts:1:1
    1 import * as assert from 'assert'
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

lib/bloom/index.ts:44:5 - error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'typeof internal' has no compatible call signatures.

44     assert(Buffer.isBuffer(e), 'Element should be Buffer')
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  lib/bloom/index.ts:1:1
    1 import * as assert from 'assert'
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

lib/evm/memory.ts:41:5 - error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'typeof internal' has no compatible call signatures.

41     assert(value.length === size, 'Invalid value size')
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  lib/evm/memory.ts:1:1
    1 import * as assert from 'assert'
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

lib/evm/memory.ts:42:5 - error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'typeof internal' has no compatible call signatures.

42     assert(offset + size <= this._store.length, 'Value exceeds memory capacity')
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  lib/evm/memory.ts:1:1
    1 import * as assert from 'assert'
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

lib/evm/memory.ts:43:5 - error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'typeof internal' has no compatible call signatures.

43     assert(Buffer.isBuffer(value), 'Invalid value type')
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  lib/evm/memory.ts:1:1
    1 import * as assert from 'assert'
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.

Is this intended?

This does minimally mean that we have to release these changes as a major v2.0.0 version to play well with existing code bases and applied configs, do I get this right?

@dryajov
Copy link
Contributor Author

dryajov commented Jun 29, 2019

My feeling for TypeScript behavior is still limited and I still have some difficulties grasping the implications of these changes.

Is there anything specific that you are not understanding?

When I add e.g. the new esModuleInterop to true setting to the current VM config, I get the following errors on compilation:

I'll check those out.

This does minimally mean that we have to release these changes as a major v2.0.0 version to play well with existing code bases and applied configs, do I get this right?

This might be the case, yes. This essentially "fixes" the import syntax and releasing as a major version bump makes total sense.

Aside from making this changes in the base config, we can also limit them to the specific PR for now, and gradually apply them to the remaining projects.

@holgerd77 is there anything missing from my explanation in ethereumjs/ethereumjs-devp2p#51 (comment)? I'd be happy to elaborate wherever it's lacking - please let me know.

Also happy to jump on a call and walk you through the changes, let me know what works best for you.

@alcuadrado
Copy link
Member

@alcuadrado Currently LGTM is set to be applied to just all repos I wouldn't want to change to manual repo selection since this would mean significant extra configuration work. I we stumble upon this more often we can nevertheless change mid-term.

Makes sense.

Is this intended?

Those errors are actually right. The right way to import assert is with import assert from "assert".
If you use a star import, the thing you get is a "module" object, not a function, so you can't call it. I like this explanation about it: https://itnext.io/great-import-schism-typescript-confusion-around-imports-explained-d512fc6769c2

This flags changes how TS interprets the imports, to make them more aligned with other systems like Babel. It is now included in the default tsconfig.json generated by tsc --init.

This does minimally mean that we have to release these changes as a major v2.0.0 version to play well with existing code bases and applied configs, do I get this right?

Releasing it as 2.0.0 is probably good. It will avoid some headaches for libraries consuming this project, as some imports have to be changed. If I'm correct, this doesn't imply a change in projects consuming those libraries. Am I right @dryajov ?

@ryanio
Copy link
Contributor

ryanio commented May 19, 2020

I think this PR is good to go.

The LGTM failure does not seem specific to this PR - also getting the same failures in #26 and #27 with No JavaScript or TypeScript code found.

I would like to merge this update so I can open a new PR that renames the package as per #26 (comment). (edit: PR opened at #27)

@ryanio ryanio merged commit 215d5f0 into ethereumjs:master May 19, 2020
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