Skip to content
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

Add network change events #1336

Merged
merged 3 commits into from
May 8, 2023
Merged

Add network change events #1336

merged 3 commits into from
May 8, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 6, 2023

Description

The network controller now emits the events networkWillChange and networkDidChange when the network is being refreshed. The event networkWillChange is emitted before the switch begins (before the network status is cleared), and the networkDidChange event is emitted after the new provider is setup (but before it has finished initializing).

Changes

  • ADDED: The events networkWillChange and networkDidChange are emitted during setProviderType, setActiveNetwork, resetConnection, and rollbackToPreviousProvider
    • The networkWillChange event is emitted before the network is switched (before the network status is cleared),
    • The networkDidChange event is emitted after the new provider is setup (but before it has finished initializing).

References

Closes #1210

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@Gudahtt

This comment was marked as resolved.

@Gudahtt Gudahtt force-pushed the add-network-change-events branch from 3b4a484 to 5f810be Compare May 8, 2023 14:14
@Gudahtt Gudahtt force-pushed the refactor-refresh-network-test-cases branch from e9fec1b to 27b0eab Compare May 8, 2023 16:19
@Gudahtt Gudahtt force-pushed the add-network-change-events branch from 5f810be to 38afc7c Compare May 8, 2023 16:42
@Gudahtt Gudahtt force-pushed the refactor-refresh-network-test-cases branch from 821d791 to afb95de Compare May 8, 2023 17:06
@Gudahtt Gudahtt force-pushed the add-network-change-events branch from 38afc7c to c579b1a Compare May 8, 2023 17:13
@Gudahtt Gudahtt force-pushed the refactor-refresh-network-test-cases branch from e8fed4e to 7553660 Compare May 8, 2023 17:28
@Gudahtt Gudahtt force-pushed the add-network-change-events branch from c579b1a to 3126c92 Compare May 8, 2023 17:29
Base automatically changed from refactor-refresh-network-test-cases to main May 8, 2023 17:49
@Gudahtt Gudahtt force-pushed the add-network-change-events branch 2 times, most recently from caf4790 to afe64b2 Compare May 8, 2023 18:48
@Gudahtt Gudahtt marked this pull request as ready for review May 8, 2023 18:50
@Gudahtt Gudahtt requested a review from a team as a code owner May 8, 2023 18:50
@Gudahtt
Copy link
Member Author

Gudahtt commented May 8, 2023

I did originally intend for this PR to include the same coverage as the extension (in that we would ensure the networkDidChange event occurred after clearing state and after setting up the provider but before the network lookup)., but that quickly became complicated. I think it would be better to revisit that later as part of #1197, so that we can consider ways to cover that functionality without overly complicating the tests that we have.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just had a few minor notes.

Comment on lines +2367 to +2374
const networkWillChange = await waitForPublishedEvents({
messenger,
eventType: 'NetworkController:networkWillChange',
operation: () => {
// Intentionally not awaited because we're capturing an event
// emitted partway through the operation
controller.rollbackToPreviousProvider();
},
});

expect(networkWillChange).toStrictEqual([[]]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: In a previous commit I added a toBeFulfilled matcher to make some of the existing tests in this repo more clear. Since the payload of the published event doesn't matter, I wonder if that matcher would be useful here too?

Suggested change
const networkWillChange = await waitForPublishedEvents({
messenger,
eventType: 'NetworkController:networkWillChange',
operation: () => {
// Intentionally not awaited because we're capturing an event
// emitted partway through the operation
controller.rollbackToPreviousProvider();
},
});
expect(networkWillChange).toStrictEqual([[]]);
const promiseForNetworkWillChange = waitForPublishedEvents({
messenger,
eventType: 'NetworkController:networkWillChange',
operation: () => {
// Intentionally not awaited because we're capturing an event
// emitted partway through the operation
controller.rollbackToPreviousProvider();
},
});
expect(promiseForNetworkWillChange).toBeFulfilled();

Similar for the other tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I might handle this in a follow-up PR though, since it applies to a lot of tests not covered in this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here: #1347

mcmire

This comment was marked as duplicate.

The network controller now emits the events `networkWillChange` and
`networkDidChange` when the network is being refreshed. The event
`networkWillChange` is emitted before the switch begins (before the
network status is cleared), and the `networkDidChange` event is emitted
after the new provider is setup (but before it has finished
initializing).

Closes #1210
@Gudahtt Gudahtt force-pushed the add-network-change-events branch from afe64b2 to 5ed561a Compare May 8, 2023 20:07
@Gudahtt Gudahtt merged commit 1dfeb29 into main May 8, 2023
@Gudahtt Gudahtt deleted the add-network-change-events branch May 8, 2023 20:48
Gudahtt added a commit that referenced this pull request May 8, 2023
The `toBeFulfilled` matcher is now used for any network controller test
where we want to ensure a promise is resolved, but we don't care about
the value.

This is a follow-up to a suggestion made in #1336
Gudahtt added a commit that referenced this pull request May 9, 2023
The `toBeFulfilled` matcher is now used for any network controller test
where we want to ensure a promise is resolved, but we don't care about
the value.

This is a follow-up to a suggestion made in #1336
Gudahtt added a commit that referenced this pull request May 9, 2023
The `toBeFulfilled` matcher is now used for any network controller test
where we want to ensure a promise is resolved, but we don't care about
the value.

This is a follow-up to a suggestion made in #1336
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The network controller now emits the events `networkWillChange` and
`networkDidChange` when the network is being refreshed. The event
`networkWillChange` is emitted before the switch begins (before the
network status is cleared), and the `networkDidChange` event is emitted
after the new provider is setup (but before it has finished
initializing).

Closes #1210
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The `toBeFulfilled` matcher is now used for any network controller test
where we want to ensure a promise is resolved, but we don't care about
the value.

This is a follow-up to a suggestion made in #1336
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The network controller now emits the events `networkWillChange` and
`networkDidChange` when the network is being refreshed. The event
`networkWillChange` is emitted before the switch begins (before the
network status is cleared), and the `networkDidChange` event is emitted
after the new provider is setup (but before it has finished
initializing).

Closes #1210
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The `toBeFulfilled` matcher is now used for any network controller test
where we want to ensure a promise is resolved, but we don't care about
the value.

This is a follow-up to a suggestion made in #1336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update NetworkController to emit networkWillChange and networkDidChange
2 participants