Fixed issues around reentrancy during external transitions #3677
Conversation
🦋 Changeset detectedLatest commit: c99f608 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
👇 Click on the image for a new way to code review
Legend |
|
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 c99f608:
|
Andarist
force-pushed
the
andarist/test-utils-entry-exit-tracking
branch
from
981efb7
to
4eb8ea6
Compare
| @@ -725,7 +727,8 @@ describe('entry/exit actions', () => { | |||
| 'exit: a.a1', | |||
| 'exit: a', | |||
| 'enter: a', | |||
| 'enter: a.a1' | |||
| 'enter: a.a1', | |||
| 'enter: a.a1.a11' | |||
There was a problem hiding this comment.
This has regressed in #3424
Andarist
force-pushed
the
andarist/fix-reentrancy
branch
from
191430f
to
6125d9f
Compare
Andarist
force-pushed
the
andarist/fix-reentrancy
branch
from
6125d9f
to
eb1b717
Compare
| ]); | ||
| }); | ||
|
|
||
| it('should reenter leaf state during its self-transition', () => { |
There was a problem hiding this comment.
This test and the other ones added below were already working. I was trying to "break" the existing implementation for those cases but they seem to work fine. They might overlap with some existing tests but I've figured out that it's better to have more than fewer tests so I've committed them 😜
| ) { | ||
| transition.entrySet.push(sn); | ||
| entrySet.add(sn); | ||
| } | ||
| } | ||
| for (const sn of prevConfig) { |
There was a problem hiding this comment.
I think that the logic in this for-of loop is fragile but I couldn't make it break right now. Perhaps it's because nodes in the prevConfig are already sorted in a way that allows this for-of loop to do its job.
| @@ -647,6 +648,7 @@ describe('entry/exit actions', () => { | |||
| expect(flushTracked()).toEqual([ | |||
| 'exit: A.A2.A2_child', | |||
| 'exit: A.A2', | |||
| 'exit: A', | |||
There was a problem hiding this comment.
So strange; I wonder why these were expected to be omitted (not exited) originally. What was our stance on whether to exit the root node or not?
There was a problem hiding this comment.
So strange; I wonder why these were expected to be omitted (not exited) originally.
It wasn't expected. When working on #3424 I've focused only on one side of the reentrancy (re-entering) but I've missed that I didn't actually fix the other half of the story - that the reentered state has to exit first.
What was our stance on whether to exit the root node or not?
In v4 we can exit the root state, in v5 this should become impossible (or at least that's what we've decided in the past months).
We didn't have an explicit issue open about those fixed issues. Those are things that I've spotted when investigating some issue reports and when reviewing some other PR.
The issue with the targeted ancestor not being exited correctly when it was already being entered during reentering transition has been mentioned in the second point here:
#3613 (comment)