-
Notifications
You must be signed in to change notification settings - Fork 3
Enable and cleanup the signupPass dom test #203
Conversation
Travis automatic deployment: Storybook book automatic deployment: |
Travis automatic deployment: Storybook book automatic deployment: |
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.
Added a couple of messages but not found any blocker, feel free to merge. I have added two commits, let me know if something comes up.
@@ -115,3 +115,10 @@ async function watchAccountAndTransactions( | |||
|
|||
return account; // resolved when first account is loaded | |||
} | |||
|
|||
export function shutdownSequence(_: any, getState: () => RootState): 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.
@ethanfrey Why this first argument?
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.
Unless I am wrong I would do:
export function shutdownSequence(state: RootState): void {
...
and just call it like: shutdownSequence(store.getState())
What do you think?
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 left this here, so it can be called like a thunk (eg. dispatch(shutdownSequence)
will work). Outside of tests, we rarely have access to the state
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.
👍
src/sequences/boot.ts
Outdated
|
||
export function shutdownSequence(_: any, getState: () => RootState): void { | ||
const connections = getConnections(getState()); | ||
for (const conn of Object.values(connections)) { |
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.
What about:
Object.values(connections).map(conn => conn.disconnect())
?
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.
or maybe even better forEach
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 like that with forEach
beforeEach(() => { | ||
store = aNewStore(); | ||
}); | ||
afterEach(() => { |
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.
Much nicer... I like this 👍
Only got a verbal approval from Adolfo on slack, so I have to use my super-cow-powers to perform an illegal merge. |
Travis automatic deployment: Storybook book automatic deployment: |
This should allow adolfo's first dom test
Checks first box in #121