seems like a good idea

Hmm, wouldn't this be the correct type? It's one or the other, not necessarily both

We can avoid using any by using our hasProperty and isObject functions from @metamask/utils:

Nice, will update accordingly!

Looping back: Why is the suggestion preferred, actually? In this case it doesn't give us any further benefits wrt types or guarantees from the type system (I think?), and the ?. is a lot easier to read. If it's just because "any bad", I'd actually think it's actually intended for situations like this? Unless I'm indeed overlooking something!

(Looks like this part will still be rewritten anyway, though, as consequence of properly fixing types)

any will disable type checking completely, anywhere the value set to any is used (including any derived types). It can easily "infect" the rest of the codebase in subtle ways, preventing type checking where we might otherwise expect it. I don't think there are any situations where we found any to be the best choice for a parameter/value/etc (the only "non-harmful" case we found was in generic type constraints, where it can't spread any further).

See more on recommended escape hatches here: https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md#escape-hatches

Though in this case, it does add type safety. This statement wasn't typed before, now it is.

I see your point about ?. being a lot easier to read. This has been one of my biggest frustrations with TypeScript, that it makes it difficult and verbose to interact with data structures that you don't know the contents of ahead of time. Any reference to an unrecognized property causes complaint, you need to first hold TypeScript's hand through the process of looking for that property first (hence the hasProperty type guard). There is probably a better way of doing this that I'm not aware of.

Def agree in the general, but here it's "enclosed" and the line has the same out-and-in types as those 6 lines...

Regardless of approach, the equivalent in my currently preferred approach over this one becomes a bit less lengthy: https://github.com/MetaMask/metamask-extension/pull/24496/files#diff-b9515eba7ccebd6c2cb33172f8f348816351594c12a5eebe825a573c9b8f59b0R22

This could make the test more flaky, as we're introducing two new async steps in between finding the element and asserting. The element could be found, then removed from the DOM or had its contents changed.

While true, if this is the case, wouldn't that be indicative of an actual error we do want to fail (ideally consistently, since that should never happen right?)

But anyway, I can take a look at if this can work again with only a single async like before.

It's true that the type of page behavior that would trigger flakiness in this case is probably undesirable.

But we do have UI that is meant to refresh (e.g. gas estimates, balances, the activity list, etc.). And we have a lot of janky UI that rerenders when it shouldn't, causing flakiness in cases like this, where it would be helpful for the test to assert something about the UI behavior even in the face of this problem.

Hmm. Tricky problem. Maybe you're right that having tests that are less-tolerant of jankyness would encourage us to fix the jankyness. But given the current state of our e2e test suite, my intuition is that it would do more harm than good in the short/medium term because we don't have capacity to do a good job of fixing janky UI everywhere right now, and we desperately need our tests to be less flaky. But I am definitely open to being convinced otherwise here.

Hmm, it's not clear what could have necessitated this change

Leftover from WIP attempts, I think. Will see if these can be reverted to keep the changes relevant.

Nit: We should be able to avoid a cast here as well, by using hasProperty and isObject from @metamask/utils