Use TypeScript; update deps; refactor exports #58

mcmire · 2022-04-01T22:29:14Z

(BREAKING) Change index so that it exports all public modules as
named exports
(BREAKING) Update all dependencies so that we get TypeScript
versions
Convert implementation code to TypeScript
Convert tests to TypeScript, and add tests to ensure that the
JavaScript-only checks still work for as much backward compatibility
as possible
Add a working TypeDoc configuration file in the event that we want to
generate documentation

typedoc.json

mcmire · 2022-04-01T22:32:06Z

src/types.ts

+ * 1. for which Infura has released official, production support ({@see {@link https://docs.infura.io}})
+ * 2. which support the JSON-RPC 2.0 protocol
+ */
+export type InfuraJsonRpcSupportedNetwork =


Not sure if it's worth listing all of the possible networks Infura takes, as we don't use them. However there shouldn't be anything in this list for which JSON-RPC is not supported.

mcmire · 2022-04-01T22:44:16Z

src/types.ts

@@ -0,0 +1,31 @@
+import type { JsonRpcRequest } from 'json-rpc-engine';


This file just has types in it. When yarn build is run, a types.d.ts file will be generated which has these types — all good — but a types.js will also be generated which is empty. I feel like this is how it's supposed to work, but it seems a bit weird to me. Not sure if there's another way we would prefer doing this or we've done this sort of thing before.

Also, Istanbul thinks that there is no test coverage for this file. At the moment this is not breaking anything because I've lowered the coverage thresholds, but in the future I guess we should tell Istanbul to ignore this whole file?

Could we make this a .d.ts file? That would get skipped by coverage steps, and would not generate an empty .js file.

Hmm, I don't think so. Then dist/createInfuraMiddleware.d.ts tries to import ./types, which is not included in the bundle, and I think that would cause errors for people.

I meant in-place, like src/types.d.ts. But I see that that also wouldn't get included in the output. Hmm. We could add a build step to copy it over to the dist directory, but that seems a little janky. It would work though.

Otherwise, including this file as an empty JS module in the output, and skipping coverage for this file, seems OK to me. Not ideal but I don't see a great way of handling this right now.

* **(BREAKING)** Change `index` so that it exports all public modules as named exports * **(BREAKING)** Update all dependencies so that we get TypeScript versions * Convert implementation code to TypeScript * Convert tests to TypeScript, and add tests to ensure that the JavaScript-only checks still work for as much backward compatibility as possible * Add a working TypeDoc configuration file in the event that we want to generate documentation

mcmire · 2022-04-08T22:02:52Z

This is now rebased and ready for re-review.

.eslintrc.js

Gudahtt · 2022-04-13T13:40:29Z

Lots going on in this PR. Maybe the TypeDoc change can be extracted to a separate PR? Seems pretty distinct from the TypeScript conversion - everything else here looks like it has closer ties.

mcmire · 2022-04-14T18:07:58Z

@Gudahtt Sure, I can extract the TypeDoc stuff.

mcmire · 2022-04-26T18:09:06Z

Ready for re-review.

Gudahtt · 2022-04-26T23:08:11Z

package.json

+    "rimraf": "^3.0.2",
+    "ts-jest": "^26.3.0",
+    "ts-node": "^10.7.0",
+    "typedoc": "^0.22.13",


Leftover from when typedoc was extracted from this PR I assume

Suggested change

"typedoc": "^0.22.13",

Fixed in e7617b4.

tsconfig.json

Gudahtt · 2022-04-26T23:11:21Z

tsconfig.json

@@ -0,0 +1,17 @@
+{
+  "compilerOptions": {
+    "allowJs": true,


Is this still needed? And likewise for skipLibCheck

Nope and nope. Removed in f045643.

Actually, for skipLibCheck, if I take that away, then when I run yarn build, I get errors like:

node_modules/eth-json-rpc-middleware/dist/block-cache.d.ts:1:37 - error TS2307: Cannot find module 'eth-block-tracker' or its corresponding type declarations. 1 import { PollingBlockTracker } from 'eth-block-tracker'; ~~~~~~~~~~~~~~~~~~~ node_modules/eth-json-rpc-middleware/dist/block-ref-rewrite.d.ts:1:37 - error TS2307: Cannot find module 'eth-block-tracker' or its corresponding type declarations. 1 import { PollingBlockTracker } from 'eth-block-tracker'; ~~~~~~~~~~~~~~~~~~~ node_modules/eth-json-rpc-middleware/dist/block-ref.d.ts:1:37 - error TS2307: Cannot find module 'eth-block-tracker' or its corresponding type declarations. 1 import { PollingBlockTracker } from 'eth-block-tracker'; ~~~~~~~~~~~~~~~~~~~

Looking at the dependencies for eth-json-rpc-middleware, it doesn't seem that eth-block-tracker is listed there. It's listed in devDependencies instead, but I don't think that's right. So that would be need to be fixed first it seems.

PR to fix this: MetaMask/eth-json-rpc-middleware#125

Bumped eth-json-rpc-middleware to 8.1.0 so build errors should no longer be present.

tsconfig.json

tsconfig.build.json

src/types.ts

Gudahtt

I thought we settled on kebab-case.js as the convention for filenames for our team. We haven't gone back to rename all older modules yet, but I was planning to at some point.

Edit: updated to fix the name of this convention - it's kebab-case not snake_case.

mcmire · 2022-04-27T16:20:32Z

@Gudahtt Hmm, I wasn't aware there was a convention. I know there are some other modules that use snake case, but the controllers repo does not, so I thought snake case was an old convention borrowed from the extension. It also seems that (based on code I've seen) the JavaScript community prefers camelCase and PascalCase over snake-case. I'm happy to change this though if we want to use snake case.

mcmire · 2022-04-27T17:16:46Z

Right, kebab case. I knew what you meant :)

mcmire · 2022-04-27T17:18:30Z

Files changed to kebab case in 253fce8.

src/fetchConfigFromReq.ts

src/create-infura-middleware.ts

Gudahtt · 2022-04-27T17:48:34Z

src/create-provider.ts

@@ -0,0 +1,25 @@
+import { JsonRpcEngine } from 'json-rpc-engine';
+import { providerFromEngine } from 'eth-json-rpc-middleware';
+import { SafeEventEmitterProvider } from 'eth-json-rpc-middleware/dist/utils/cache';


Hmm. Not ideal relying upon a type that isn't exported. We should probably update this library to export this.

Good point. I'll work on this. Update: issue added here MetaMask/eth-json-rpc-middleware#126

PR submitted to fix this here: MetaMask/eth-json-rpc-middleware#127

I'm pointing to the latest commit in 9cfa316 just to get the build to pass and will revert this when the above PR is merged.

Bumped eth-json-rpc-middleware to 8.1.0.

src/create-infura-middleware.ts

Gudahtt · 2022-04-28T11:59:03Z

Looks good! Aside from the one outstanding suggestion and the dep update, this seems ready to go

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

This makes the build errors go away 🎉

mcmire · 2022-05-02T15:18:49Z

Ready for review again.

Gudahtt

LGTM!

mcmire requested a review from a team as a code owner April 1, 2022 22:29

mcmire force-pushed the convert-to-typescript branch 2 times, most recently from af9e092 to a431ba3 Compare April 1, 2022 22:36

mcmire commented Apr 1, 2022

View reviewed changes

mcmire force-pushed the convert-to-typescript branch from a431ba3 to 6a510cf Compare April 1, 2022 22:42

mcmire commented Apr 1, 2022

View reviewed changes

mcmire mentioned this pull request Apr 4, 2022

Could you help remove the vulnerability introduced by package node-fetch? #50

Closed

mcmire force-pushed the convert-to-jest branch 2 times, most recently from a5d8aa0 to 49a7466 Compare April 6, 2022 16:55

mcmire force-pushed the convert-to-typescript branch from f15ddec to 669f999 Compare April 6, 2022 17:04

Base automatically changed from convert-to-jest to main April 6, 2022 20:26

mcmire force-pushed the convert-to-typescript branch from 669f999 to b405c0f Compare April 8, 2022 21:59

Gudahtt reviewed Apr 13, 2022

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

mcmire self-assigned this Apr 21, 2022

mcmire added 3 commits April 26, 2022 11:56

Change TypeDoc out directory to docs/

390c71c

Consolidate the JS-only tests and TS tests

c05ed66

Remove typedoc config

a2ffca7

Gudahtt reviewed Apr 26, 2022

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 26, 2022

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 26, 2022

View reviewed changes

tsconfig.build.json Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 27, 2022

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 27, 2022

View reviewed changes

mcmire added 4 commits April 27, 2022 11:17

Use kebab-case for file names

253fce8

Remove typedoc package

e7617b4

Copy tsconfig.json and tsconfig.build.json from module template

f045643

Remove invalid subdomains from InfuraJsonRpcSupportedNetwork

5a4e03f

Gudahtt reviewed Apr 27, 2022

View reviewed changes

mcmire added 2 commits April 27, 2022 12:22

Remove extra '| undefined'

aae8e3e

Remove extra type parameter

82352d1

mcmire mentioned this pull request Apr 27, 2022

Add export for SafeEventEmitterProvider MetaMask/eth-json-rpc-middleware#126

Closed

mcmire added 3 commits April 27, 2022 14:12

Test with fixed eth-json-rpc-middleware

9cfa316

Remove unused type

c953228

Fix export in create-provider.ts

05a0d16

Gudahtt reviewed Apr 28, 2022

View reviewed changes

src/create-infura-middleware.ts Outdated Show resolved Hide resolved

Update src/create-infura-middleware.ts

a644b1e

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

mcmire mentioned this pull request Apr 28, 2022

Expose SafeEventEmitterProvider type MetaMask/eth-json-rpc-middleware#127

Merged

mcmire added 2 commits May 2, 2022 11:16

Don't escape slashes

3e0e302

Bump eth-json-rpc-middleware to 8.1.0

3874efa

This makes the build errors go away 🎉

Gudahtt approved these changes May 2, 2022

View reviewed changes

mcmire merged commit ec3e88e into main May 2, 2022

mcmire deleted the convert-to-typescript branch May 2, 2022 15:43

This was referenced May 2, 2022

Standardize all the things #59

Closed

6.0.0 #60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TypeScript; update deps; refactor exports #58

Use TypeScript; update deps; refactor exports #58

mcmire commented Apr 1, 2022 •

edited

Loading

mcmire Apr 1, 2022

mcmire Apr 1, 2022

mcmire Apr 6, 2022

Gudahtt Apr 13, 2022

mcmire Apr 26, 2022

Gudahtt Apr 27, 2022

mcmire commented Apr 8, 2022

Gudahtt commented Apr 13, 2022

mcmire commented Apr 14, 2022

mcmire commented Apr 26, 2022

Gudahtt Apr 26, 2022

mcmire Apr 27, 2022

Gudahtt Apr 26, 2022

mcmire Apr 27, 2022

mcmire Apr 27, 2022

mcmire Apr 27, 2022

mcmire May 2, 2022

Gudahtt left a comment •

edited

Loading

mcmire commented Apr 27, 2022 •

edited

Loading

mcmire commented Apr 27, 2022

mcmire commented Apr 27, 2022

Gudahtt Apr 27, 2022

mcmire Apr 27, 2022 •

edited

Loading

mcmire Apr 27, 2022

mcmire May 2, 2022

Gudahtt commented Apr 28, 2022

mcmire commented May 2, 2022

Gudahtt left a comment

		@@ -0,0 +1,31 @@
		import type { JsonRpcRequest } from 'json-rpc-engine';

Use TypeScript; update deps; refactor exports #58

Use TypeScript; update deps; refactor exports #58

Conversation

mcmire commented Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire commented Apr 8, 2022

Gudahtt commented Apr 13, 2022

mcmire commented Apr 14, 2022

mcmire commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

mcmire commented Apr 27, 2022 • edited Loading

mcmire commented Apr 27, 2022

mcmire commented Apr 27, 2022

Choose a reason for hiding this comment

mcmire Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt commented Apr 28, 2022

mcmire commented May 2, 2022

Gudahtt left a comment

Choose a reason for hiding this comment

mcmire commented Apr 1, 2022 •

edited

Loading

Gudahtt left a comment •

edited

Loading

mcmire commented Apr 27, 2022 •

edited

Loading

mcmire Apr 27, 2022 •

edited

Loading