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

Bump Typescript to v4 #1924

Merged
merged 4 commits into from
Mar 8, 2021
Merged

Bump Typescript to v4 #1924

merged 4 commits into from
Mar 8, 2021

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented Jan 1, 2021

Fixes type breaking changes to be compatible with Typecript v4, so we can benefit from the performance improvements and new features.

See https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/
Fixes #1784

@github-actions github-actions bot added Api scope-networking All issues related to networking, gossip, and libp2p. labels Jan 1, 2021
@codeclimate
Copy link

codeclimate bot commented Jan 1, 2021

Code Climate has analyzed commit b573aba and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@dapplion
Copy link
Contributor Author

dapplion commented Jan 1, 2021

A lot of unit tests are failing, anyone has a clue what might have changed to cause so many failures?

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

packages/lodestar-utils/src/bytes.ts Show resolved Hide resolved
@wemeetagain
Copy link
Member

Seems the stubs are no longer working

Eg:
in lodestar-beacon-state-transition/test/unit/stateTransition/epoch/justification.test.ts
set breakpoint on L43
getCurrentEpochStub.called === false

Seems related to the getters no longer being enumerable?
microsoft/TypeScript#38568
@mpetrunic any thoughts?

@mpetrunic
Copy link
Member

Seems the stubs are no longer working

Eg:
in lodestar-beacon-state-transition/test/unit/stateTransition/epoch/justification.test.ts
set breakpoint on L43
getCurrentEpochStub.called === false

Seems related to the getters no longer being enumerable?
microsoft/TypeScript#38568
@mpetrunic any thoughts?

Typical response from Ryan...
There is nice tread on sinon repo sinonjs/sinon#2258

I think it worked before because typescript didn't generated esm compliant code (aka module imports were mutable). If my memory serves me correctly, babel was already doing that so we switched from babel-register to ts-node/register.

Few ideas:

@dapplion
Copy link
Contributor Author

dapplion commented Jan 5, 2021

We could use an import mocking library such as https://www.npmjs.com/package/rewiremock

I've used it with success and offers full type support. Example:

  const { default: getPortsToOpen } = await rewiremock.around(
      () => import("../../../src/watchers/natRenewal/getPortsToOpen"),
      mock => {
        mock(() => import("../../../src/modules/docker/listContainers"))
          .with({ listContainers })
          .toBeUsed();
      }
    );

From https://github.com/dappnode/DNP_DAPPMANAGER/blob/cbd7cbcf3e155f0dd7e9e969ee37b431fb916819/packages/dappmanager/test/watchers/natRenewal/getPortsToOpen.test.ts#L74

@mpetrunic
Copy link
Member

We could use an import mocking library such as https://www.npmjs.com/package/rewiremock

I've used it with success and offers full type support. Example:

  const { default: getPortsToOpen } = await rewiremock.around(
      () => import("../../../src/watchers/natRenewal/getPortsToOpen"),
      mock => {
        mock(() => import("../../../src/modules/docker/listContainers"))
          .with({ listContainers })
          .toBeUsed();
      }
    );

From https://github.com/dappnode/DNP_DAPPMANAGER/blob/cbd7cbcf3e155f0dd7e9e969ee37b431fb916819/packages/dappmanager/test/watchers/natRenewal/getPortsToOpen.test.ts#L74

Im pretty sure that only works because you are using typescript <3.9. All those libs were just modifying exports. Feel free to try it though

@dapplion
Copy link
Contributor Author

dapplion commented Jan 5, 2021

Im pretty sure that only works because you are using typescript <3.9. All those libs were just modifying exports. Feel free to try it though

RIght, I have not tried it with Typescript >= 3.9

dapplion added 2 commits March 7, 2021 13:43
Fix type issue in benchmark-utils


Bump @types/eventsource to support Typescrip v4

Remove un-used toHex() function


Cast pubkey to Uint8Array in lodestar-validator

Cast querystring.parse to fastify querystringParser


networkInterfaces maybe undefined


Declare Promise resolve type void


Remove unused import ArrayLike


Revert deleting toHex()
@dapplion
Copy link
Contributor Author

dapplion commented Mar 7, 2021

rewiremock works fine with Typescript v4.

  • I've skipped all tests in the beacon state transition that were mocking imports
  • I've refactored the tests in lodestar that were mocking imports with rewiremock. It's not pretty, and slow but allows us to move forward.

All our code does dependency injection everywhere, where we pass entire instances of the chain, network, etc. However, we don't do dependency injection for BLS and state transition utils. To fix this type of issues with mocking imports that result in very ugly verbose test code we could also do dependency injection for these remaining items

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

This is good for now to move forward.
I think ideally, we can build out better testing infrastructure that can help us test on valid-ish states so we can avoid relying on mocking and just have better e2e tests.

@wemeetagain wemeetagain merged commit 03f7474 into master Mar 8, 2021
@wemeetagain wemeetagain deleted the dapplion/tsv4 branch March 8, 2021 17:54
@dapplion dapplion mentioned this pull request Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope-networking All issues related to networking, gossip, and libp2p.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump Typescript to 4.1
3 participants