Backfill types for some dependencies #733

mcmire · 2022-03-18T23:14:55Z

Some of our internal NPM packages are still untyped. Currently we get
around this by stubbing the types for these modules in a
dependencies.d.ts file, but this merely tags everything exported from
the modules as any.

To address this, this commit adds proper types for most of the modules
marked in dependencies.d.ts so that these types can be accessed and
utilized within the code that uses these modules. The modules that are
left over are those that would either take too long to fill in or those
that we are planning on removing soon anyway.

NOTE: These changes are not intended to be breaking. In other words, all the types here should reflect current behavior, not ideal behavior. If you see a change that could be breaking, let me know.

Fixes #728.

mcmire · 2022-03-18T23:16:06Z

src/assets/AccountTrackerController.ts

  AccountTrackerState
 > {
-  private ethQuery: any;
+  private ethQuery: EthQuery | null;


This property can be null because it is not set in the constructor (or at least I didn't see that it was set in the constructor; I may be wrong).

src/assets/Standards/CollectibleStandards/ERC1155/ERC1155Standard.test.ts

mcmire · 2022-03-18T23:18:16Z

src/assets/TokenListController.test.ts

-  const { erc20, logo: filePath, ...token } = (contractMap as ContractMap)[
-    tokenAddress
-  ];
+  const { erc20, logo: filePath, ...token } = contractMap[tokenAddress];


Types were added in this PR for @metamask/contract-metadata, so typecasting is no longer necessary.

mcmire · 2022-03-18T23:20:08Z

src/gas/fetchBlockFeeHistory.test.ts

    const latestBlockNumber = 3;
    const numberOfRequestedBlocks = 3;

+    beforeEach(() => {


This should have been there all along and for some reason it was not causing the test to fail.

mcmire · 2022-03-18T23:21:06Z

src/gas/fetchBlockFeeHistory.ts

  includeNextBlock = false,
 }: {
-  ethQuery: EthQuery;
+  ethQuery: EthQueryish;


An "EthQueryish" is an object that looks like an EthQuery without the full gamut of methods that EthQuery supports. It's the type of the argument that query now takes, and since it only really cares that the object has a sendAsync method, that's what this type checks for.

src/network/NetworkController.ts

mcmire · 2022-03-18T23:29:47Z

src/transaction/TransactionController.ts


      const txNonce =
        nonce ||
-        (await query(this.ethQuery, 'getTransactionCount', [from, 'pending']));


These changed because there is now a new type parameter on query that governs the type of data that it returns (as unfortunately due to the generic nature of query, we can't figure it out). The behavior is the same though, these are just type annotations.

mcmire · 2022-03-18T23:32:10Z

src/util.test.ts


-  it('bNToHex', () => {
-    expect(util.BNToHex(new BN('1337'))).toBe('0x539');
+  describe('BNToHex', () => {


Updated the tests on this function because I wanted to make sure it did what I expected and I found the tests lacking.

mcmire · 2022-03-18T23:32:29Z

src/util.ts

 */
 export function weiHexToGweiDec(hex: string) {
  const hexWei = new BN(stripHexPrefix(hex), 16);
-  return fromWei(hexWei, 'gwei').toString(10);


fromWei already returns a string; this is unnecessary.

mcmire · 2022-03-19T00:02:28Z

jest.config.js

  collectCoverageFrom: ['./src/**/*.ts'],
  // TODO: Test index.ts
-  coveragePathIgnorePatterns: ['./src/index.ts'],
+  coveragePathIgnorePatterns: ['./src/index.ts', './src/gas/gas-util.ts'],


My changes to this file resulted in the coverage percentage falling below our threshold, so I've disabled this file for the time being. I logged an issue to backfill these tests here: #734

A previous commit updated the TypeScript configuration so TypeScript would better recognize type definition files whose purpose was to backfill types for existing packages. After testing, this revealed a misunderstanding of the `paths` option in `tsconfig.json`. It turns out that `paths` completely overrides how TypeScript resolves modules and locates type definition files for modules. The consequence of this option is that if a module includes type definitions, but we also supply type definitions for that module in `types/`, then our type definitions will win. This degrades the developer experience: sometimes, a package already has types, but we merely need to *augment* those types in some way. The `paths` option makes it impossible to do this. This commit keeps the `types/` directory (as it'll be important later) but informs TypeScript about the type definition files here by adding them to the `include` option in `tsconfig.json` (which was the original strategy).

Some of our internal NPM packages are still untyped. Currently we get around this by stubbing the types for these modules in a `dependencies.d.ts` file, but this merely tags everything exported from the modules as `any`. To address this, this commit adds proper types for most of the modules marked in `dependencies.d.ts` so that these types can be accessed and utilized within the code that uses these modules. The modules that are left over are those that would either take too long to fill in or those that we are planning on removing soon anyway.

mcmire · 2022-03-30T22:33:26Z

I realized there were some issues with the changes I made in #732, so this PR is now based on a new PR, #761, which addresses those issues.

mcmire · 2022-03-31T17:23:22Z

As explained in #761, I don't think adding ambient types for these packages to this repo is the right way to go about this. I think it would make more sense (and would also be more straightforward) to just add types directly to these packages (or add the types to the DefinitelyTyped repo for ones that we don't fully control). Thoughts/comments?

Gudahtt · 2022-04-13T15:10:56Z

src/assets/AccountTrackerController.ts

+    const { ethQuery } = this;
+
+    if (ethQuery === null) {
+      return;


This looks like a change in behavior. It now returns nothing instead of throwing an error.

The same goes for syncBalanceWithAddresses - it now returns an empty object rather than throwing.

mcmire · 2022-04-13T21:56:56Z

I'm still going the route of adding types to our packages directly rather than use ambient types as I think that will be better in the end, so I'll close this, and we can use this for inspiration later if we really need to revive this.

mcmire requested a review from a team as a code owner March 18, 2022 23:14

mcmire commented Mar 18, 2022

View reviewed changes

mcmire force-pushed the backfill-types branch 3 times, most recently from 633d7fb to 3929f6c Compare March 19, 2022 00:00

mcmire commented Mar 19, 2022

View reviewed changes

mcmire force-pushed the fix-tsconfig-for-tests branch 2 times, most recently from d130fde to ecd3eea Compare March 25, 2022 19:56

Base automatically changed from fix-tsconfig-for-tests to main March 30, 2022 20:06

mcmire force-pushed the backfill-types branch from 6b63e8b to 10848e4 Compare March 30, 2022 20:26

mcmire changed the base branch from main to fix-tsconfig March 30, 2022 22:25

mcmire force-pushed the backfill-types branch from 10848e4 to 995a619 Compare March 30, 2022 22:31

mcmire mentioned this pull request Mar 31, 2022

Fix inclusion of module-augmenting .d.ts's #761

Merged

Base automatically changed from fix-tsconfig to main April 5, 2022 20:35

Gudahtt reviewed Apr 13, 2022

View reviewed changes

mcmire closed this Apr 13, 2022

Uh oh!

Backfill types for some dependencies #733

Backfill types for some dependencies #733

Uh oh!

Conversation

mcmire commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcmire Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcmire Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcmire Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire commented Mar 30, 2022

Uh oh!

mcmire commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

mcmire commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mcmire commented Mar 18, 2022 •

edited

Loading

mcmire Mar 18, 2022 •

edited

Loading

mcmire Mar 18, 2022 •

edited

Loading

mcmire commented Mar 31, 2022 •

edited

Loading