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

Feature: createModel() #1439

Merged
merged 27 commits into from
Jan 15, 2021
Merged

Feature: createModel() #1439

merged 27 commits into from
Jan 15, 2021

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Sep 5, 2020

This PR adds support for an opt-in helper function called createModel(), which is meant to assist in creating the datamodel (SCXML) for the machine:

import { createMachine } from 'xstate';
import { createModel } from 'xstate/lib/model'; // opt-in, not part of main build

interface UserContext {
  name: string;
  age: number;
}

type UserEvents =
  | { type: 'updateName'; value: string }
  | { type: 'updateAge'; value: number }

const userModel = createModel<UserContext, UserEvents>({
  name: 'David',
  age: 30
});

const assignName = userModel.assign({
  name: (_, e) => e.value // correctly typed to `string`
}, 'updateName'); // restrict to 'updateName' event

const machine = createMachine<UserContext, UserEvents>({
  context: userModel.context,
  initial: 'active',
  states: {
    active: {
      on: {
        updateName: {
          actions: assignName
        }
      }
    }
  }
});

@changeset-bot
Copy link

changeset-bot bot commented Sep 5, 2020

🦋 Changeset detected

Latest commit: a4e129c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
xstate Minor

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 5, 2020

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 b04f5c8:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@johnyanarella
Copy link

johnyanarella commented Sep 5, 2020

I was just thinking about how the current uncertainty around #989 and #1367 confounds this effort. The API surface area gets less elegant when once you have to require the developer to differentiate between assign and reset.

But that begs the question... does it even make sense for the entire model shape to change (as in the reset option)? Isn't that an entirely different animal--a union type where each variant might expose different available updaters...

@davidkpiano
Copy link
Member Author

But that begs the question... does it even make sense for the entire model shape to change (as in the reset option)? Isn't that an entirely different animal--a union type where each variant might expose different available updaters...

Most of the time it doesn't, but there are usecases highlighted in those issues that demonstrate the need.

@johnyanarella
Copy link

johnyanarella commented Sep 5, 2020

Oh, for sure - definitely didn't mean to come across as dismissive of those needs at all.

Just thinking aloud today as the 💡 slowly flickered on in my head, realizing that those concerns were really about the friction of representing a union type using a key/value store. (And the SCXML spec has essentially mandated a key/value store as the root data model type.) The imperfect emulation of that, and all the effort to emulate it, falls away when storing that union type as keyed value instead. Bonus: language support starts to come into play (both in TypeScript and Swift--which has enums with associated values). Obvious in retrospect, but I was just not seeing it clearly before.

I think that means this cool new createModel() effort gets to similarly side-step those worries, too? i.e. you could have a property of the model that is a union type, and any related constraints become internal updater details rather than the responsibility of this new feature.

packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Show resolved Hide resolved
@@ -936,14 +951,18 @@ export type Assigner<TContext, TEvent extends EventObject> = (
meta: AssignMeta<TContext, TEvent>
) => Partial<TContext>;

export type PartialAssigner<
Copy link
Member

Choose a reason for hiding this comment

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

I kinda feel like it would be more appropriate to name this a PropertyAssigner and the other one a PartialAssigner 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I know... we can change this for v5

export interface TransitionConfig<TContext, TEvent extends EventObject> {
cond?: Condition<TContext, TEvent>;
actions?: Actions<TContext, TEvent>;
export type RestrictedActions<
Copy link
Member

Choose a reason for hiding this comment

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

is this used anywhere? cant find any reference to this 🤔

@@ -224,6 +231,33 @@ describe('assign', () => {
maybe: 'defined'
});
});

it('can assign from event', () => {
Copy link
Member

Choose a reason for hiding this comment

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

q: is this here just to validate types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

packages/core/src/model.ts Outdated Show resolved Hide resolved
packages/core/src/model.ts Outdated Show resolved Hide resolved
Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

This version feels better than some previous iterations with withAssigners etc 👍 Good job.

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

Please just remember about adding the appropriate changeset so the changelog entry gets correct attribution to PR etc :)

packages/core/src/model.ts Outdated Show resolved Hide resolved
TEvent extends { type: K } ? TEvent : never
>;
export type TransitionsConfigMap<TContext, TEvent extends EventObject> = {
[K in TEvent['type']]?: TransitionConfigOrTarget<TContext, TEvent, K>;
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason behind this change? 🤔 seems a little bit stranger to pass TEvent and K around and compute at the end rather than pass a computed TEvent from here

is it about TransitionConfigTarget including a StateNode target? wouldn't it be better to allow there any at the TEvent position as we should be able to target any state node in the given machine config? As this is going to be removed in v5 anyway (targeting with getters etc) I'm not sure if it's worth complicating the codebase for its sake

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is an accidental commit, thanks for catching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I just discovered one of the reasons for this change: the raise() event needs to be able to infer all possible events.

type: 'ALOHA'
}),
}) as any /* TODO: FIX */,
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I've wanted to look into it and I've checked out your branch locally but after removing those casts and comments I couldn't see any TS errors. Could you recheck on your machine? That being said - inferred generics are off and I don't understand why the inferred type is assignable here to the expected one 😭

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'll keep the comment in here for now.

@davidkpiano davidkpiano merged commit 7a83723 into master Jan 15, 2021
@davidkpiano davidkpiano deleted the davidkpiano/model branch January 15, 2021 18:00
@github-actions github-actions bot mentioned this pull request Jan 15, 2021
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

3 participants