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

waitFor fixes for cases when starting to waitFor stopped interpreter #3200

Merged
merged 3 commits into from
Apr 7, 2022

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Apr 7, 2022

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Apr 7, 2022

⚠️ No Changeset found

Latest commit: a3d308b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -429,12 +429,16 @@ export class Interpreter<
this.listeners.add(listener);

// Send current state to listener
if (this.status === InterpreterStatus.Running) {
if (this.status !== InterpreterStatus.NotStarted) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a small change in behavior. I was afraid to emit this at all times because this.initialState might be impure

@ghost
Copy link

ghost commented Apr 7, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 7, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a3d308b:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

listener(this.state);
}

if (resolvedCompleteListener) {
this.onDone(resolvedCompleteListener);
if (this.status === InterpreterStatus.Stopped) {
resolvedCompleteListener();
Copy link
Member Author

Choose a reason for hiding this comment

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

can't call this within onDone easily because there the callback expects to receive a doneInvoke event. Here we know that it's a parameter-less observer.complete though.

}
});

const service = interpret(machine).start();

setInterval(() => service.send('NEXT'), 10);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was resulting in a spammy logs coming from here:

if (this.status === InterpreterStatus.Stopped) {
// do nothing
if (!IS_PRODUCTION) {
warn(
false,
`Event "${_event.name}" was sent to stopped service "${
this.machine.id
}". This service has already reached its final state, and will not transition.\nEvent: ${JSON.stringify(
_event.data
)}`
);
}
return this.state;
}

as this interval was never disposed. So I've just changed this to a timeout that doesn't suffer from this problem.

Comment on lines -74 to -76
setTimeout(() => {
throw new Error('Should not reach here');
}, 50);
Copy link
Member Author

Choose a reason for hiding this comment

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

similarly, this was never disposed and could be thrown when other test was executing and thus fail that other test. I don't think this was adding that much here and by using await expect().rejects() we make sure that this waitFor throws

@Andarist Andarist merged commit 56c0a36 into davidkpiano/waitFor Apr 7, 2022
@Andarist Andarist deleted the andarist/waitFor-fixes branch April 7, 2022 12:37
Andarist added a commit that referenced this pull request Apr 7, 2022
* Add missing changeset about changes in the core from #3200

* Update .changeset/red-walls-lay.md

Co-authored-by: David Khourshid <davidkpiano@gmail.com>

Co-authored-by: David Khourshid <davidkpiano@gmail.com>
@github-actions github-actions bot mentioned this pull request Apr 7, 2022
nevilm-lt pushed a commit to nevilm-lt/xstate that referenced this pull request Apr 22, 2022
…ter (statelyai#3200)

* Fixed `waitFor` hanging till timeout with final state matching the predicate

* Fixed `waitFor` hanging till timeout with final state not matching the predicate

* Fixed spammy sends in the waitFor test file
nevilm-lt pushed a commit to nevilm-lt/xstate that referenced this pull request Apr 22, 2022
…tatelyai#3203)

* Add missing changeset about changes in the core from statelyai#3200

* Update .changeset/red-walls-lay.md

Co-authored-by: David Khourshid <davidkpiano@gmail.com>

Co-authored-by: David Khourshid <davidkpiano@gmail.com>
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.

None yet

2 participants