[v5] Restrict `context` to object by davidkpiano · Pull Request #2294 · statelyai/xstate

davidkpiano · 2021-06-10T02:56:01Z

This PR restricts context passed into createMachine(...) or createModel(...) to be an object.

Closes https://github.com/davidkpiano/xstate/projects/1#card-40598611

changeset-bot · 2021-06-10T02:56:03Z

🦋 Changeset detected

Latest commit: 5cd04ee

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

This PR includes changesets to release 9 packages

Name	Type
xstate	Major
@xstate/examples-fetch	Patch
@xstate/graph	Major
@xstate/immer	Major
@xstate/inspect	Major
@xstate/react	Major
@xstate/svelte	Major
@xstate/test	Major
@xstate/vue	Major

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

VanTanev · 2021-06-10T15:25:30Z

In typescript, object | undefined basically means "anything." All these pass:

type X = object | undefined

let a: X = []
let b: X = new Map()
let c: X = new String('a')
let d: X = new Number(1)
let e: X = new Number(NaN)
let f: X = new Boolean(false)
// ... etc

What is the restriction you're trying to apply?

davidkpiano · 2021-06-10T16:24:45Z

In typescript, object | undefined basically means "anything." All these pass:
type X = object | undefined

let a: X = []
let b: X = new Map()
let c: X = new String('a')
let d: X = new Number(1)
let e: X = new Number(NaN)
let f: X = new Boolean(false)
// ... etc
What is the restriction you're trying to apply?

Only literal objects { ... } or undefined. I did this late at night 😅 Let me know if there's a better type for this.

VanTanev · 2021-06-10T17:45:01Z

Unfortunately, as far as I know, there is no such type.
This is because TS has no concept of a hash/map literal as a separate type.

I think that usually Record<string, any> | undefined communicates the idea better, but all the above examples pass the same way. I /think/ that there were some esoteric cases where it provided slightly better results than object, but can't remember off the top of my head.

Andarist · 2021-06-10T21:44:33Z

Yep, this is a very unfortunate (and quite frankly - misleading) quirk of TS. Some simple examples showcased here. I think the object reads best and since I'm not sure when Record would give us anything beyond object I think we can stick with it to keep things simple. And actually object might be better in some situations (see here)

I wonder though if we have to support undefined - or rather: what should be the result of this:

const m = createMachine<undefined>({})
m.context // {} or undefined?

I think that defaulting to an empty object could potentially lead to less special-casing certain code paths in the implementation and that there is no much value in supporting undefined contexts.

davidkpiano · 2021-06-13T02:31:50Z

packages/core/src/scxml.ts

-    ${scope}
-    ${body}
+${scope}
+with (context) {


Asking for forgiveness for using the deprecated with statement, but ✨ it works so well ✨

davidkpiano · 2021-06-13T02:33:04Z

packages/core/test/machine.test.ts

+    it('machines defined without context should have a default empty object for context', () => {
+      const machine = createMachine({});
+
+      expect(machine.initialState.context).toEqual({});
+    });


❕ New test

davidkpiano · 2021-06-13T02:33:30Z

packages/core/test/machine.test.ts

      });

-      expect(changedBarMachine.initialState.context).toBeUndefined();
+      expect(changedBarMachine.initialState.context).toEqual({ bar: 42 });


❕ Changed test outcome (context is no longer undefined ever)

davidkpiano · 2021-06-13T02:33:44Z

packages/core/test/types.test.ts

+describe('types', () => {
+  it('defined context in createMachine() should be an object', () => {
+    createMachine({
+      // @ts-expect-error
+      context: 'string'
+    });
+  });
+
+  it('defined context passed to createModel() should be an object', () => {
+    // @ts-expect-error
+    createModel('string');
+  });
+});


❕ New tests

davidkpiano · 2021-06-13T02:35:32Z

Reviewers: you can safely ignore most of the changes in these files (there's a ton), because they're mostly changing the types to enforce that TContext is of type MachineContext.

Look for the comments with an ❕ icon.

packages/core/src/State.ts

packages/core/src/StateNode.ts

packages/xstate-graph/src/graph.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

Restrict context to object or undefined

4f803e3

davidkpiano changed the base branch from main to next June 10, 2021 02:56

davidkpiano added 2 commits June 12, 2021 17:31

Restrict context to only object, fix SCXML tests

c25c4da

Refactor context types

eb0fc68

davidkpiano changed the title ~~[v5] Restrict context to object or undefined~~ [v5] Restrict context to object Jun 13, 2021

Fix tests

f8b2869

davidkpiano commented Jun 13, 2021

View reviewed changes

Add changeset

c0a6dca

Andarist reviewed Jun 13, 2021

View reviewed changes

packages/core/src/State.ts Show resolved Hide resolved

Andarist reviewed Jun 13, 2021

View reviewed changes

packages/core/src/StateNode.ts Outdated Show resolved Hide resolved

Andarist reviewed Jun 13, 2021

View reviewed changes

packages/xstate-graph/src/graph.ts Outdated Show resolved Hide resolved

davidkpiano and others added 3 commits June 13, 2021 10:23

Update packages/core/src/StateNode.ts

0d59cf5

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

Update packages/xstate-graph/src/graph.ts

7b0c44e

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

Add default context

5cd04ee

Andarist approved these changes Jun 13, 2021

View reviewed changes

davidkpiano merged commit 4b32de7 into next Jun 13, 2021

github-actions bot mentioned this pull request Jun 13, 2021

Version Packages (alpha) #902

Merged

Andarist deleted the v5/context-merging branch March 22, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[v5] Restrict `context` to object #2294

[v5] Restrict `context` to object #2294
davidkpiano merged 8 commits intonextfrom
v5/context-merging

davidkpiano commented Jun 10, 2021 •

edited

Loading

Uh oh!

changeset-bot bot commented Jun 10, 2021 •

edited

Loading

Uh oh!

VanTanev commented Jun 10, 2021 •

edited

Loading

Uh oh!

davidkpiano commented Jun 10, 2021

Uh oh!

VanTanev commented Jun 10, 2021

Uh oh!

Andarist commented Jun 10, 2021

Uh oh!

davidkpiano Jun 13, 2021

Uh oh!

davidkpiano Jun 13, 2021

Uh oh!

davidkpiano Jun 13, 2021

Uh oh!

davidkpiano Jun 13, 2021

Uh oh!

davidkpiano commented Jun 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

davidkpiano commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

VanTanev commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidkpiano commented Jun 10, 2021

Uh oh!

VanTanev commented Jun 10, 2021

Uh oh!

Andarist commented Jun 10, 2021

Uh oh!

davidkpiano Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

davidkpiano Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

davidkpiano Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

davidkpiano Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

davidkpiano commented Jun 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidkpiano commented Jun 10, 2021 •

edited

Loading

changeset-bot bot commented Jun 10, 2021 •

edited

Loading

VanTanev commented Jun 10, 2021 •

edited

Loading