-
Notifications
You must be signed in to change notification settings - Fork 394
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
Strictly Explicit: Enable TypeScript strict mode #333
Conversation
5cfc1f6
to
50ee484
Compare
To do this, disable some subparts of two linter rules that required the component prop types to be optional if they were mentioned in defaultProps. In older versions of TypeScript, declaring a prop required when there was a default provided did not allow callers to omit the prop correctly when using the component. More recent versions of TypeScript (3+) now correctly expose an optional property externally while treating it as required inside the body of the component. All defaultProps-mentioned properties are now marked required in prop types, laying the groundwork for enabling strict mode.
The ECMAScript ?. notation is used to invoke the event handlers only when the event handlers are passed. defaultProps that are set with an empty handler are eliminated.
In strict mode, Record<string, unknown> and string aren't substitutable for the type Redirect to takes; instead, more precision is required. To achieve this, pull in the history package as a direct (rather than transitive) dependency and use its History.LocationDescriptor type to match react-router-dom's expectations.
Strict mode leads to some issues with the current structure, including the fact that T extends unknown; these adjustments require a slightly different syntactic approach.
For now these are unknowns as we aren't actually selecting seeds yet, but the lack of explicit typing was leading to issues in strict mode.
These are miscellaneous adjustments that either deal with possible undefineds via optional chaining (?.) or plug in explicit fallback text where otherwise an error or undefined might have been at play.
These are cases where type incompatibilities appeared once all strict mode requirements were enabled.
Deleting global.crypto isn't possible because it's not optional on the global object. Instead, restore the original global.crypto value in the afterEach phases.
This mostly involved changing mode-specific adjustments to deal with the fact that the mode can be undefined, which then cascaded into a webpack version compatibility issue with the CopyPlugin.
This involved two changes: Emittery's primary type includes a `debug` property that is locked to the event types in strict mode, so the adjusted Emittery type used in the Service interface now omits that property. Additionally, the started() type was using an implicitly any `this` type that wasn't flagged by noImplicitAny but is flagged by noImplicitThis when strict mode is flipped on. The type now correctly references the type of the containing class.
AnyEVMBlock represents the fact that not all blocks are EIP1559 blocks. Existing code assumed this was true, something which never errored as strict null checks were required to detect the issues with it. Strict mode will make these errors, and AnyEVMBlock improves expectations across the board to match this correct behavior.
At the type level, the big differences are the optionalization of the `to` field (which is not specified in contract-creating transactions), the addition of the gasLimit field (which exists for all transaction types, including those that are confirmed), the conversion of all transaction types from `interface` to `type` declarations, and the renaming of the `gas` field to `gasUsed` for clarity. This last one is a mismatch to the default JSON-RPC name, but is clearer for internal use. Additionally, the concept of an almost-signed-confirmed transaction vs a confirmed transaction, which existed solely due to an expectation that confirmed transactions had no gas limit, is gone. In its place is the concept of an almost-signed transaction, whose signature fields are optional. Last but not least, a variety of places, especially in external-to-internal type conversion, were refined to reflect proper null/undefined type handling, and to reflect updated field names and types.
This includes a new function, jtdValidatorFor, in the validation.ts file. This function lazily instantiates a validator in a fully-typed way, and handles compilation failures for the schema in a way that is compatible with standard Ajv validation failures (otherwise the lazy facade would fail).
To do this, a new method jsonSchemaValidatorFor is introduced. It works the same way as jtdValidatorFor, but uses JSON Schema. The Coingecko price JTD schema is adapted to a JSON schema that correctly captures the underlying types, and the prices.ts consumers properly handle the possibility of an undefined price for a given asset pair. The Coingecko price schema is odd in that it has a field that is a number (last_updated_at) alongside an arbitrary number of string keys that are either a number or undefined. Due to a bug in Ajv's typing, the JSON schema needs a little massaging in the type system to properly fit into the expectations; this is detailed alongside the schema in a comment.
These are cases where null or undefined values could be guarded against either using optional chaining, using a simple empty alternate, or using conditional type narrowing.
For strict initialization, all fields must either be declared as possibly-null or they must be assigned directly in the constructor. To do this, the saved redux state is now resolved in the `create` function (since it requires a promise) and the store is initialized and wrapped directly in the Main constructor. The `initializeRedux` method remains, but simply acts as a kickoff point to connect the various services to the redux store.
Since the `to` field is no longer required in transactions (to accommodate contract creation transactions), the account lookups used in the transactionConfirmed and transactionSeen reducers need to deal with that possibility.
TypeScript generally doesn't infer tuple types, and with strict mode enabled a type like (number | PricePoint)[] can't be mapped through a function with expecting a type like `[number, PricePoint]`. Instead, the original map is adjusted to declare the tuple type, which can then be used later via inference.
This mostly consists of using `any` in cases where we were using `unknown` in order to avoid issues with the heterogenous nature of the action handlers in the alias container for webext-redux.
requireUnlocked is now correctly sync (as it has no async components), and #cachedKey usage is adjusted to properly reflect `requireUnlocked`'s guarantees. The outcome is still a little sloppy, but for now there's no clear way to make `requireUnlocked` help TypeScript infer that the this.#cachedKey property is known-non-null.
Because JS allows throwing any object, the assumption that thrown objects are always Error instances isn't correct.
The forms here are cases where null was being compared to undefined, null was being returned instead of undefined, or null/undefined had to be declared explicitly as types in tests (due to strict mode being off).
In cases where there is no block hash, don't try to look up the block data.
The blocknative object is left null if there is no Blocknative API key. There is an additional question of whether the extension should be able to operate without a functioning Blocknative connection; for now, the answer is assumed to be yes, without the associated data, and without a fallback.
Not all browsers provide a stack trace in all circumstances, so stacks are only logged when the error stack is defined.
🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊 🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊🙈🙉🙊
This was a case where unit prices could multiply out to a non-integer when adjusting numbers to a BigInt fixed-point representation. When this happened, the BigInt conversion failed and stopped all price handling. The number is now truncated to prevent this.
50ee484
to
c789691
Compare
As with #330, I've requested review from everyone who has touched code
Additionally, since we talked about it as a team, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK and comfortable with most of the diff, but would like one more 👍 before merge.
@@ -48,7 +48,7 @@ interface Props { | |||
|
|||
export default function OnboardingVerifySeed(props: Props): ReactElement { | |||
const { triggerPreviousStep } = props | |||
const [isSelected, setIsSelected] = useState([]) | |||
const [isSelected, setIsSelected] = useState<unknown[]>([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these supposed to be string[]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're nothing right now, so I typed it accordingly. They will be… Whatever they will be once Henry finishes implementing :) I expect string though, yeah.
beforeEach(() => { | ||
// polyfill the WebCrypto API | ||
global.crypto = webcrypto as unknown as Crypto | ||
}) | ||
|
||
afterEach(() => { | ||
delete global.crypto | ||
global.crypto = originalCrypto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -143,7 +143,7 @@ export default class KeyringService extends BaseService<Events> { | |||
this.emitKeyrings() | |||
} | |||
|
|||
private async requireUnlocked(): Promise<void> { | |||
private requireUnlocked(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make some API design stuff interesting. Sync + async methods came up a lot dealing with the legacy keyring API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what you mean tbh. requireUnlocked
doesn't do any async stuff, but async functions can still call it normally without changing their async status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MM keyring API used an odd mishmash of sync and async signatures that made typing hard. Part of why we jettisoned it. There are still some spots in hd-keyring
that are like this though... declared async
just because a caller expects it.
Anyway, more a note for future me than you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm at war with unnecessary async. Since we control the full stack I'd like to eliminate any mistyping like that sooner rather than later.
@@ -560,7 +560,7 @@ export default class ChainService extends BaseService<Events> { | |||
tx: AnyEVMTransaction, | |||
dataSource: "local" | "alchemy" | |||
): Promise<void> { | |||
let error: Error | |||
let error: unknown = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option here was to do an instanceof
guard in the catch
es below… I picked the least potentially lossy thing in this case, but def open to adjusting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That emoji was more existential than critical :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i understand and share your pain haha. Just wanted to further explain inline 😁
return { | ||
hash: gethResult.hash, | ||
blockHeight: gethResult.number, | ||
parentHash: gethResult.parentHash, | ||
difficulty: gethResult.difficulty && BigInt(gethResult.difficulty), | ||
// FIXME Hold for ethers/v5.4.8 _difficulty BigNumber field; the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is painful lawd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really is 😭
chainID: "1", | ||
nonce: | ||
if ( | ||
typeof options.from !== "undefined" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm being dense, but... why can't we check their value directly (!== undefined
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some weird thing in my brain about not doing this and it is because… undefined
is mutable 🙃 This is not true in a global context (see the MDN article on undefined
), so I wouldn't expect a library to be able to hit us with an unexpected behavior here… ESLint will also catch it if we do it ourselves…
That said, I feel like maybe it's still good practice to stick to this? <_< It spooks me that it can be reassigned, though I don't love coding by spookiness lol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, just realised that this was something that we used in our js obfuscator tool ... :)
.filter((b) => b.error === null && b.tokenBalance !== null) | ||
.filter( | ||
( | ||
b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indentation is horrendous, I hope the linter made you do it 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One step further, the linter actually did it for me lol.
There's a world where we introduce a couple of convenience type guards that handle non-null, non-undefined, and non-null-or-undefined... But there weren't as many places where it was needed as I expected, so I stayed away from factoring it out for now.
// The type returned by Ajv validator functions, but without the schemaEnv | ||
// property. Our code does not use it, and its non-optional and Ajv-internal | ||
// nature makes our lazy wrapping difficult to implement correctly. | ||
type EnvlessValidateFunction<T> = ((json: unknown) => json is T) & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woof
@@ -85,9 +85,9 @@ function findClosestAsset( | |||
|
|||
function prunePrices(prices: PricePoint[]): PricePoint[] { | |||
// TODO filter prices to daily in the past week, weekly in the past month, monthly in the past year | |||
const pricesToSort = prices.map((pp) => [pp.time, pp]) | |||
const pricesToSort = prices.map<[number, PricePoint]>((pp) => [pp.time, pp]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
Leaving types off as was previously happening leads to `any` everywhere, but evidently not in a way that noImplicitAny catches. This is a pitfall that probably needs to be addressed systemically, but for now improve the typing---and fix a related bug where setShowingActivityDetail was being invoked with `undefined` and breaking the activity detail view.
@@ -58,7 +58,7 @@ function genericLogger(level: LogLevel, input: unknown[]) { | |||
|
|||
console.log(...input) | |||
|
|||
const stackTrace = new Error().stack.split("\n").filter((line) => { | |||
const stackTrace = new Error().stack?.split("\n")?.filter((line) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is much simpler than your previous attempt, I like it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some notes for the issues I had w/ the validation
the review is not yet complete but wanted to start communicating these
|
||
/** | ||
* TODO create the proper organisation for the validation when using it to validate anything else | ||
* a good starting point would a base validation class in /lib and | ||
* a validation instance in every service | ||
*/ | ||
const ajv = new Ajv() | ||
const ajvJTD = new AjvJTD() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this instantiation be lazy too?
I had some progress w/ a getter based solution but it's far from perfect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%. Realized that after and didn't want to bloat the PR further, but would love a follow-on PR that makes this lazy. Just a matter of wrapping it in a function, basically.
The result of the validation function is always a plain boolean, while errors and related metadata is assigned to the compiled validation function itself. The lazy wrapper now correctly copies these fields to itself after a validation run, instead of copying no properties from the boolean result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a question for the history dependency but other than that this PR seems to be golden!
Congrats on this huge axe swinging! I would have died under the weight of this task (or would have went bald from pulling my hair out :) )
@@ -27,6 +27,7 @@ | |||
"@tallyho/tally-background": "0.0.1", | |||
"classnames": "^2.3.1", | |||
"dayjs": "^1.10.6", | |||
"history": "^4.9.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see only the typing part is used from this package.
Do you see this package as being useful in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually transitively pulled in by react-router
and react-router-dom
(see here), but we have a lint rule that requires us to explicitly declare the dependency if we reference it in the code.
hasTabBar?: boolean | ||
hasTopBar?: boolean | ||
hasTabBar: boolean | ||
hasTopBar: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These props can become required without changing CorePage
's usage in the pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because defaultProps
are defined! Typescript treats them as required in the function, but callers see them as optional because of their presence on defaultProps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! I wasn't aware of this
This PR enables TypeScript strict mode, which adds a bunch of stricter type checks to
the compilation process, including the one that caused the most prominent changes,
strictNullChecks
, that requires us to account fornull
andundefined
explicitly.Two big things to note:
particular, we're relying on our libraries to have correct types declared. If
those libraries don't themselves use strict mode, they can often omit undefined
and null in their type signatures, and our code is none the wiser. This
specifically happened with ethers, which currently returns a
null
for blockdifficulty due to Rise in ethereum difficulty will break block.difficulty ethers-io/ethers.js#2001 , on a
property that has no null possibility declared.
null/undefined than I planned, and I would probably have made it more than one
PR if I'd known. As it is the history is a little too intertwined to pull out
though.
As with #330, I'll post a separate comment drawing attention to the pieces that
are most relevant to each person I've tagged for review!