Deep initials #1041
Deep initials #1041
Conversation
🦋 Changeset is good to goLatest commit: b24e47b We got this. 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 |
|
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 b24e47b:
|
|
I believe this will conflict with mine in the The transition definition's Can you compare it to that branch? I'll make a PR for that soon |
| const mutStatesToEnter = new Set<StateNode<TContext, any, TEvent>>(); | ||
| const mutStatesForDefaultEntry = new Set<StateNode<TContext, any, TEvent>>(); | ||
|
|
||
| computeEntrySet( |
There was a problem hiding this comment.
this is quite hacky but solves the problem of resolving initial states recursively with the existing algorithm
i would like to refactor this in the future, but for the time being it seems OK - @davidkpiano's work on representing initial states as transitions will affect this anyway, so that would be a good time to refactor this
| stateKey: string | ||
| ): StateNode<TContext, any, TEvent> { | ||
| stateKey: string, | ||
| discardInvalidNodes: boolean = false |
There was a problem hiding this comment.
I don't like this flag at all, will look into removing it in the future. This has been introduced solely to pass this test:
https://github.com/davidkpiano/xstate/blob/797a53ad88767f649580628380bbdf86e9fc8a0c/packages/core/test/invalid.test.ts#L50
I'm not convinced if we should bother with this within core algorithms - it would be way easier if we could just assume valid values/configurations. If one migrates their schema they should be responsible for resolving conflicts - especially because we can't resolve all of them and a decision on how to migrate removed regions would be better done on the developer's side.
There was a problem hiding this comment.
@davidkpiano thoughts on this one? should we bother "fixing" invalid inputs in the core?
There was a problem hiding this comment.
We shouldn't, you're right. Let's assume valid values/configs.
| }); | ||
| } else { | ||
| getEffectiveTargetStates( | ||
| { target: resolveHistoryTarget(s) } as TransitionDefinition<TC, TE>, |
There was a problem hiding this comment.
this is also a hack, as the only thing actually needed by getEffectiveTargetStates is the target property, but this will be refactored once we add a default transition to history states, that can be done in a followup PR after both this PR and the microstep work lands on the next branch
| @@ -243,7 +241,6 @@ export function getValue<TC, TE extends EventObject>( | |||
| configuration: Configuration<TC, TE> | |||
| ): StateValue { | |||
| const config = getConfiguration([rootNode], configuration); | |||
There was a problem hiding this comment.
im not sure why this accepts 2 arguments, i've removed the "prevStateNodes" one and all tests pass without it, should I remove it in a followup PR?
| @@ -86,7 +86,6 @@ describe('algorithm', () => { | |||
| 'a', | |||
| 'b1', | |||
| 'c1', | |||
| 'c2', | |||
There was a problem hiding this comment.
I've changed the implementation of the getConfiguration slightly and this was the only test that has failed in the result. As mentioned in another comment - I don't know why getStateNodes accepts prevNodes. What's the exact purpose of getConfiguration? I use it to resolve initial nodes & ancestors of input nodes.
There was a problem hiding this comment.
@davidkpiano would love your commentary on that
There was a problem hiding this comment.
The previous state nodes are needed if only a partial configuration was given, e.g., if a compound node was given and not the child nodes, we can determine the child nodes from the previous config.
There was a problem hiding this comment.
So is this intended as the public API? Because as far as I can tell such a situation never happens internally.
There was a problem hiding this comment.
Not public API. Can safely be removed.
No description provided.