Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Fix prod tsconfig and include ethjs-util types in dist #260

Merged
merged 3 commits into from
Jul 7, 2020
Merged

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Jul 2, 2020

Fixes two bugs introduced in #248.

  • restores include option in tsconfig.prod.json. Files are no longer written to dist/src and dist/test is no longer produced
  • renames @types/ethjs-util/index.d.ts --> index.ts. This forces the compiler to generate a .d.ts file for it and copy everthing into dist appropriately. (More info about this approach on stack overflow here.)

@coveralls
Copy link

coveralls commented Jul 2, 2020

Coverage Status

Coverage remained the same at 99.711% when pulling 173e6bc on fix/dist into 584cc82 on master.

@holgerd77
Copy link
Member

@cgewecke thanks for addressing this so quickly, this looks already a lot better with the correct general folder structure and the types from ethjs-util being exported.

When I pack and test-install this I don't get access to an ES6-style import structure though (or is this technically just not possible?) nor do I get any type information, see the following screenshot:

Bildschirmfoto 2020-07-06 um 11 41 34

Could you have a look and state what should and should not be working here? 😄

We should also add usage information on the docs here along (on this PR or the subsequent release PR).

@holgerd77
Copy link
Member

@nebojsa94 @alcuadrado we need to address this here until we can release v7.0.3.

@cgewecke
Copy link
Contributor Author

cgewecke commented Jul 6, 2020

@holgerd77 Very sorry, thanks for double-checking this.

I think 173e6bc fixes. Tested locally and TS was able to resolve the types from the dist. Have also removed the added compiler options that were papering over this problem in the tests.

We should also add usage information

The existing example shows an import statement like this:

import { isValidChecksumAddress, unpad, BN } from 'ethereumjs-util'

Are you thinking of something more detailed?

@@ -1,3 +1,4 @@
/// <reference path="./@types/ethjs-util/index.ts"/>
Copy link
Member

@alcuadrado alcuadrado Jul 6, 2020

Choose a reason for hiding this comment

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

I think you can just do an import "./@types/ethjs-util/index.ts"; here. I'm not sure if this triple-slash reference would work after compilation, as that index.ts won't exist in the build.

Copy link
Contributor Author

@cgewecke cgewecke Jul 6, 2020

Choose a reason for hiding this comment

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

@alcuadrado

Yeah this is weird but tsc auto-generates the contents of the @types folder in the dist creating a .d.ts file.

dist/
  @types/
    ethjs-util/
      index.d.ts
      index.js
      index.js.map

And it rewrites the triple slash reference in the dist's main index.d.ts like this:

/// <reference path="@types/ethjs-util/index.d.ts" />

Copy link
Member

Choose a reason for hiding this comment

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

🤯

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

This looks good now, thanks, tested again by doing a fresh build and local test install, ES6 import and type hints are now working! 👍

@cgewecke the cited example with import { isValidChecksumAddress, unpad, BN } from 'ethereumjs-util' is actually still not working, but not due the changes here but due to unpad not available as a function any more, hehe. 😄

But no worry, will give this an update on the release PR.

@holgerd77 holgerd77 merged commit bd4ce85 into master Jul 7, 2020
@holgerd77 holgerd77 deleted the fix/dist branch July 7, 2020 11:25
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