Implicitly Existential: Add noImplicitAny to TypeScript config, fix related issues #330

Shadowfiend · 2021-10-13T19:14:07Z

This PR enables the noImplicitAny rule in our TypeScript config:

In some cases where no type annotations are present, TypeScript will fall back to a type of any for a variable when it cannot infer the type.

This can cause some errors to be missed, for example:
function fn(s) {
  // No error?
  console.log(s.subtr(3));
}
fn(42);
Turning on noImplicitAny however TypeScript will issue an error whenever it would have inferred any:
function fn(s) {
            ^ Parameter 's' implicitly has an 'any' type.
  console.log(s.subtr(3));
}

This required adding typing to a few places, and tightening up types in a few others. It
revealed a few issues with the type structures we were expecting in the WalletActivity*
components, partly related to capitalization and partly to type support for placeholders
we weren't using anymore.

In a few very specific cases types are forced using as. In general these are related to
very specific issues that can be fixed in the future, and are marked with FIXME.

Draft until I double-check that functionality remains solid.

This will help us avoid errors about untyped packages once noImplicitAny is enabled. Also bump our webpack version to align types to most of the webpack versions referenced in the type packages.

These were from a previous iteration of db code, and are not currently in use.

This is more reflective of what we're working with, and will ensure we don't start accessing properties willy nilly.

We still need to improve validation to ensure the API gives us data that fits the specified types, but this gets us to avoiding anys.

This matches existing uses in the code that were coming in as anys and avoiding type checking.

This takes care of good-enough typing for the nested function parameters.

This is used in the UI slice as well as the activity detail/listing components.

These were dangling, leading to missing types all the way down the selector tree.

noImplicitAny will frown upon our existing approach, so introduce a new one that preserves types all the way down.

The new approach validates the response at the highest level and then translates those types down into the remaining data handling.

Likely enabling strict mode will stop these from being resolved as any implicitly, but in the meantime type them explicitly.

These now explicitly check for the decimals fields that we expect exist.

Due to the fact that most webpack plugin type packages depend on specific webpack versions, and the way plugin signatures are resolved from minor version to minor version breaks, some forced casting is included to get to a compiling file while avoiding implicit anys.

The typing is generic rather than tied to ActivityItem, and allows us to ensure all the pieces are properly typed and the map correctly mentions properties that are valid for the object being adapted for UI display.

We ended up with some implicit anys and unexpected unknowns due to the JDT schema's inability to type a few properties. With noImplicitAny, this triggers actual errors, and we adjust for those here.

The day is here! Implicit anys are no longer allowed, and TypeScript will error on them. Explicit anys should also be avoided, but for now that's still treated as a warning.

The ethers logger was in use, making for confusing output.

When an Ethereum token price has more decimals than the fiatDecimals, BigInt would receive a floating-point number and error out; this commit adds a call to Math.trunc to truncate further precision (not round!).

Shadowfiend · 2021-10-13T20:34:18Z

I've requested review from everyone who has touched code related to this. Here are some
specific callouts for folks:

@henryboldi most of the UI typing changes weren't super heavy, but in addition to generally eyeballing I'll call your attention to bf11816, which adds an ActivityItem type that is used throughout the whole activity details stack. I also fully typed the renameAndPickKeys helper @ 5a07d30 and tightened some checks up in the USD value multiplications @ 931c44c .
@itsrachelfish I changed the logger stuff to use unknown types @ 1da3e45 . This isn't strictly something that noImplicitAny flags, but in general our default should be unknown instead of any, because any lets you look up properties without giving you any guidance or errors, while unknown flags as an error any attempt to access specific properties on the object (since we don't know if they exist).
@minutuslausus I had to make a couple of small adjustments to the price validator typing @ a943599 ; in particular, adding the as const changed last_updated_at from an unknown type to a number type.
@mhluongo I tweaked how we typed the asset transfer stuff @ 1e1f46f, and also removed a bunch of dead code related to older db migrations that was all any-typed.

Please everyone feel free to look at much more than the specific commits listed here 🙏

greg-nagy · 2021-10-14T05:41:12Z

ui/components/Wallet/WalletActivityDetails.tsx

+    detailTransformer: (value: T[P]) => string
+  }
+}
+


just a quick note that I really like this code and typing 💯

I don't like single letter variables 😢

mhluongo

A couple questions, but all for merging. Thanks for taking out the trash here!

mhluongo · 2021-10-15T21:27:35Z

package.json

@@ -85,7 +88,7 @@
    "terser-webpack-plugin": "^5.1.1",
    "ts-loader": "^9.2.3",
    "typescript": "^4.3.2",
-    "webpack": "^5.39.0",
+    "webpack": "^5.58.1",


Ser. Seems like a Webpack bump should make it into the commit message 😄

This bump is to the version that resolves anyway (due to the ^), I just tried to be more explicit to see if I could deal with webpack @types tomfoolery (it didn't work).

mhluongo · 2021-10-15T21:30:09Z

ui/components/Core/CorePage.tsx

@@ -19,7 +19,7 @@ export default function CorePage(props: Props): ReactElement {
  const [isNotificationsOpen, setIsNotificationsOpen] = useState(false)
  const [isDevToolsOpen, setIsDevToolsOpen] = useState(false)

-  function handleOpenHiddenDevMenu(e) {
+  function handleOpenHiddenDevMenu(e: React.MouseEvent) {


Man I struggle with React types

mhluongo · 2021-10-15T21:34:48Z

background/tests/json-serialization.test.ts

@@ -28,11 +28,11 @@ test("round-trips mixed values with bigints correctly", () => {
    float: 132.167,
    exp: 12.683e13,
    string: "hello",
-    nullCheck: null,
+    nullCheck: null as null, // FIXME Fix when strict is enabled.


I don't get this one?

Without strictNullChecks, null results in an implicit any, because null cannot be inferred as a type. strictNullChecks makes null and undefined full types in the type system, so the explicit cast to the type is no longer required.

(Great reminder, I forgot to hit these in the strict mode branch lol.)

mhluongo · 2021-10-15T21:35:46Z

background/redux-slices/accounts.ts

@@ -403,12 +403,13 @@ export const selectAccountAndTimestampedActivities = createSelector(
          asset.symbol === assetItem.asset.symbol && asset.recentPrices.USD
      )

-      if (rawAsset) {
-        // Does this break if the token is less than 1 USD? Hah...


Isn't this still an open question? Seems like we still want some sort of a TODO here

Bah. Yeah, I originally changed this to always assume the same side had USD on it, but then found that even though the docs promise it, the reality does not seem to match that promise. No surprise there, tbh <_<

Actually we have access to each side of the pair's symbol here… Feels both type-able correctly and more easily detectable here. Added a TODO in the strict mode branch.

mhluongo · 2021-10-15T21:37:20Z

ui/components/Wallet/WalletActivityDetails.tsx

+    detailTransformer: (value: T[P]) => string
+  }
+}
+


mhluongo · 2021-10-15T21:38:46Z

background/lib/alchemy.ts

@@ -194,7 +195,8 @@ export async function getTokenBalances(
  if (!isValidAlchemyTokenBalanceResponse(json)) {
    logger.warn(
      "Alchemy token balance response didn't validate, did the API change?",
-      json
+      json,
+      isValidAlchemyTokenBalanceResponse.errors


mhluongo

Go go go 🚀

Shadowfiend added 11 commits October 13, 2021 14:52

Specify a couple of reduce types to avoid anys

1488eba

Add a few type packages

4c4711a

This will help us avoid errors about untyped packages once noImplicitAny is enabled. Also bump our webpack version to align types to most of the webpack versions referenced in the type packages.

Drop unused state/db/migration templates

38a5ace

These were from a previous iteration of db code, and are not currently in use.

Move logger.ts anys to unknowns

1da3e45

This is more reflective of what we're working with, and will ensure we don't start accessing properties willy nilly.

Improve typing for prices.ts getEthereumTokenPrices

898377f

We still need to improve validation to ensure the API gives us data that fits the specified types, but this gets us to avoiding anys.

Ensure convertToEth can also take bigints directly

9e4e350

This matches existing uses in the code that were coming in as anys and avoiding type checking.

Specify reduxCache middleware type

a409b10

This takes care of good-enough typing for the nested function parameters.

Introduce a unified ActivityItem type for the UI

bf11816

This is used in the UI slice as well as the activity detail/listing components.

Specify get*State selector types

b62362f

These were dangling, leading to missing types all the way down the selector tree.

Declare React event types in handlers

b629bb6

Clean up background async thunk typing

2fdd13d

noImplicitAny will frown upon our existing approach, so introduce a new one that preserves types all the way down.

Shadowfiend requested review from mhluongo, itsrachelfish, henryboldi and greg-nagy October 13, 2021 19:14

Shadowfiend force-pushed the implicitly-existential branch from 81df7e0 to b0f5ba1 Compare October 13, 2021 19:21

Shadowfiend added 13 commits October 13, 2021 16:31

Adjust asset transfer schema validation to avoid anys

1e1f46f

The new approach validates the response at the highest level and then translates those types down into the remaining data handling.

Drop a type for placeholder data that is no longer relevant

01269a6

Type a couple of redux hooks

50238cc

Fix a couple of test types that are resolved as implicit any

fe9ebe9

Likely enabling strict mode will stop these from being resolved as any implicitly, but in the meantime type them explicitly.

Tighten type guards for account/timestamp activities

931c44c

These now explicitly check for the decimals fields that we expect exist.

Add typing for the renameAndPickKeys helper

52710db

The typing is generic rather than tied to ActivityItem, and allows us to ensure all the pieces are properly typed and the map correctly mentions properties that are valid for the object being adapted for UI display.

checkIfPlausibleETHAddress now has a type for its parameter!

a14fb41

Deal with Coingecko price validator's loose JDT typing

107f42b

We ended up with some implicit anys and unexpected unknowns due to the JDT schema's inability to type a few properties. With noImplicitAny, this triggers actual errors, and we adjust for those here.

Enable noImplicitAny in tsconfig

c54ccec

The day is here! Implicit anys are no longer allowed, and TypeScript will error on them. Explicit anys should also be avoided, but for now that's still treated as a warning.

Include Alchemy JSON validation errors when warning about failure

4c7d2ed

Use Tally logger in alchemy.ts

0df26e3

The ethers logger was in use, making for confusing output.

Avoid an error with long-decimal token prices

91561bd

When an Ethereum token price has more decimals than the fiatDecimals, BigInt would receive a floating-point number and error out; this commit adds a call to Math.trunc to truncate further precision (not round!).

Shadowfiend force-pushed the implicitly-existential branch from b0f5ba1 to 91561bd Compare October 13, 2021 20:32

greg-nagy reviewed Oct 14, 2021

View reviewed changes

Shadowfiend marked this pull request as ready for review October 14, 2021 12:52

henryboldi previously approved these changes Oct 14, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into implicitly-existential

ff7679e

mhluongo previously approved these changes Oct 15, 2021

View reviewed changes

Merge branch 'main' into implicitly-existential

813666d

mhluongo dismissed stale reviews from henryboldi and themself via 813666d October 15, 2021 21:40

mhluongo enabled auto-merge October 15, 2021 21:40

mhluongo approved these changes Oct 15, 2021

View reviewed changes

mhluongo merged commit 3d6e62e into main Oct 15, 2021

mhluongo deleted the implicitly-existential branch October 15, 2021 21:45

Shadowfiend mentioned this pull request Oct 19, 2021

Strictly Explicit: Enable TypeScript strict mode #333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implicitly Existential: Add noImplicitAny to TypeScript config, fix related issues #330

Implicitly Existential: Add noImplicitAny to TypeScript config, fix related issues #330

Shadowfiend commented Oct 13, 2021

Shadowfiend commented Oct 13, 2021

greg-nagy Oct 14, 2021

mhluongo Oct 15, 2021

itsrachelfish Oct 18, 2021

mhluongo left a comment

mhluongo Oct 15, 2021

Shadowfiend Oct 19, 2021

mhluongo Oct 15, 2021

mhluongo Oct 15, 2021

Shadowfiend Oct 15, 2021

Shadowfiend Oct 15, 2021

mhluongo Oct 15, 2021

Shadowfiend Oct 15, 2021

Shadowfiend Oct 15, 2021 •

edited

Loading

mhluongo Oct 15, 2021

mhluongo Oct 15, 2021

mhluongo left a comment

Implicitly Existential: Add noImplicitAny to TypeScript config, fix related issues #330

Implicitly Existential: Add noImplicitAny to TypeScript config, fix related issues #330

Conversation

Shadowfiend commented Oct 13, 2021

Shadowfiend commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhluongo left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhluongo left a comment

Choose a reason for hiding this comment

Shadowfiend Oct 15, 2021 •

edited

Loading