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

Removed the model overload from createMachine #2657

Conversation

mattpocock
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Sep 15, 2021

🦋 Changeset detected

Latest commit: 51c0b6c

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

This PR includes changesets to release 1 package
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

@@ -69,7 +55,7 @@ export function createMachine<
// Ensure that only the first overload matches models, and prevent
// accidental inference of the model as the `TContext` (which leads to cryptic errors)
config: TContext extends Model<any, any, any, any>
? never
? 'Passing typeof model to the first generic of createMachine is no longer supported - use model.createMachine instead'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Andarist will this work as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, there are now only two overloads left in this file. They look very similar in signature - can we condense them into... only... ONE OVERLOAD...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

export function createMachine<
  TContext,
  TEvent extends EventObject = AnyEventObject,
  TTypestate extends Typestate<TContext> = { value: any; context: TContext }
>(
  // Ensure that only the first overload matches models, and prevent
  // accidental inference of the model as the `TContext` (which leads to cryptic errors)
  config: TContext extends Model<any, any, any, any>
    ? 'Passing typeof model to the first generic of createMachine is no longer supported - use model.createMachine instead'
    : MachineConfig<TContext, any, TEvent>,
  options?: Partial<MachineOptions<TContext, TEvent>>
): StateMachine<TContext, any, TEvent, TTypestate>;
export function createMachine<
  TContext,
  TEvent extends EventObject = AnyEventObject,
  TTypestate extends Typestate<TContext> = { value: any; context: TContext }
>(
  config: MachineConfig<TContext, any, TEvent>,
  options?: Partial<MachineOptions<TContext, TEvent>>
): StateMachine<TContext, any, TEvent, TTypestate> {
  return new StateNode<TContext, any, TEvent, TTypestate>(
    config,
    options
  ) as StateMachine<TContext, any, TEvent, TTypestate>;
}

Copy link
Member

Choose a reason for hiding this comment

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

Note that createMachine has only a single overload here now - the second thing is like a private declaration. If possible you could still consolidate both though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha - it's not possible to consolidate both and still keep the pretty error message.

"xstate": minor
---

Removed the ability to pass a model as a generic to `createMachine`, in favour of `model.createMachine`. This lets us cut an overload from the definition of `createMachine`, meaning errors become more targeted and less cryptic.
Copy link
Member

Choose a reason for hiding this comment

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

This was already being implemented in https://github.com/statelyai/xstate/pull/2518/files#diff-00f777398251992f42d7061fc6a79e0c6e7c8aebfd257cb1d8c04129c84e0273R51 and accepted back then so I think it's an OK change.

@Andarist
Copy link
Member

@mattpocock this test should be removed now:

it('should not compile if missing context with plain createMachine(...)', () => {
const toggleModel = createModel({ count: 0 });
// @ts-expect-error
createMachine<typeof toggleModel>({
id: 'machine',
initial: 'inactive',
// missing context:
// context: toggleModel.initialContext,
states: {
inactive: {}
}
});
});

@mattpocock
Copy link
Contributor Author

@Andarist good spot, removed

@mattpocock mattpocock merged commit 2fbd43e into statelyai:main Sep 15, 2021
This was referenced Sep 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

4 participants