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

[Beta-release] useMachine #241

Merged
merged 31 commits into from
Jun 10, 2020
Merged

[Beta-release] useMachine #241

merged 31 commits into from
Jun 10, 2020

Conversation

LevelbossMike
Copy link
Owner

@LevelbossMike LevelbossMike commented May 4, 2020

Fixes #235

Todos:

  • add a test for useMachine(this.args.machine) and figure out how to handle this - right now use would resetup the interpreter which probably isn't something useful
  • same as ^ but for local attributes passed to withConfig or withContext
  • add a test that makes sure service is cleaned up properly when parent gets destroyed

Releases on npm to try this out:

  • 0.10.0-beta.0

@NullVoxPopuli
Copy link
Contributor

Tryin' it out: NullVoxPopuli/emberclear#803

createUsable(context, { machine }) {
const owner = getOwner(context);

const interpreter = new InterpreterService(machine);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should check if this is a Machine or MachineConfig and create a Machine, if machine is a MachineConfig (to eliminate the need for side-effecting code in module-space -- as the XState docs have by default (cause it's easier to teach that way))


setup() {
this.service = interpret(this.machine, {
clock: {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this clock here? / what's it for?

Copy link
Owner Author

@LevelbossMike LevelbossMike May 7, 2020

Choose a reason for hiding this comment

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

https://xstate.js.org/api/classes/interpreter.html#clock

The clock that is responsible for setting and clearing timeouts, such as delayed events and transitions.

We are making sure that we use the ember runloop when the xstate-interpreter schedules delays, transitions et al. This is important when we are testing etc.

get state() {
return {
currentState: this.currentState,
send: this.service.send,
Copy link
Contributor

Choose a reason for hiding this comment

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

what about transition?

Copy link
Owner Author

Choose a reason for hiding this comment

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

this.service refers to an interpreted machine - i.e. a xstate-Interpreter. In contrast to a Machine an Interpreter has no transition-method only a send-method. You just send events to an interpreter and the interpreter then figures out if it needs to transition into a new state.

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli May 7, 2020

Choose a reason for hiding this comment

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

I think I had just gotten confused with remembering my own code. I have a transition component action that just calls send

@LevelbossMike
Copy link
Owner Author

Can you add this to the type def, too?

will try - not entirely sure how to type this yet but I'll do my best

@pangratz
Copy link
Collaborator

Overall looks awesome, I'll give this a try on the weekend 🚀

},
"devDependencies": {
"@ember/optional-features": "^1.3.0",
"@ember/render-modifiers": "^1.0.2",
"@glimmer/component": "^1.0.0",
"@glimmer/tracking": "^1.0.0",
Copy link
Collaborator

@pangratz pangratz May 17, 2020

Choose a reason for hiding this comment

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

Is moving this to dependencies intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we're good with dropping support for pre 3.15, I'd assume so ;)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I moved it because we use it in the usable. 🤔 will recheck

CHANGELOG.md Outdated Show resolved Hide resolved

get state() {
return {
currentState: this.currentState,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we name this state it's aligned to xstate/react and xstate/vue

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just.. current? since the getter is already state? we have the mental context that we're already getting the "state", but current would just mean the current version of it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

If we name this state it's aligned to xstate/react and
xstate/vue

I was thinking the same as I saw that 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, ignore me. I thought this was used in a different way. I'm 👍 on state

addon/index.js Outdated
return function () {
return {
get() {
return xstateMatchesState(state, this[statechartPropertyName].currentState.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is still the only thing that doesn't sit right with me -- mostly because it's relying on an implicit property name.

I wonder if it makes sense to do something like:

class Foo {
  @use myInterpreter = useMachine(machineStuff);

  @matches(this.myInterpreter, 'someState');
  isSomeState;
}

idk.

Copy link
Owner Author

@LevelbossMike LevelbossMike May 17, 2020

Choose a reason for hiding this comment

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

This is how this works already.

class Foo {
  @use myInterpreter = useMachine(myMachine);

  @matchesState('busy', 'myInterpreter')
  isBusy;
}

If you leave out the default property it checks the statechart-property. I think best is to make this this default property name configurable via a setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if possible to pass a 'this' property to a decorator, that would make the most sense intuitively, but I don't know if the decorators are evaluated before instantiation.

My gut on this is that matchesState is for a very imperitive style of consuming statecharts, and I think we should survey some people outside of the xstate community for feedback :/

Copy link
Owner Author

@LevelbossMike LevelbossMike May 17, 2020

Choose a reason for hiding this comment

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

I don't follow what you mean by imperative style for consuming. matchesState is reactive getter for matching against state patterns - it's just a decorator that makes it more ergonomical (arguably) to match against the current-state of the interpreter.

You could also do this when you wanted to react to state changes but @matchesState saves you some typing.

// @matchesState is literally doing the same thing
get isBusy() {
  return matchesState('busy', this.statechart.state.value);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow what you mean by imperative style for consuming.

Just that there is a magic, undeclared relationship with the property name of the useMachine value.

You could also do this when you wanted to react to state changes but @matchesState saves you some typing.

I think that's more inline with modern JS. Though, I'd switch the args:

get isBusy() {
  return matchesState(this.statechart.state.value, 'busy');
}

or even:

get isBusy() {
  return this.statechart.state.match('busy');
}

@LevelbossMike
Copy link
Owner Author

@NullVoxPopuli @pangratz updated PR with tests for assign-usage and reactivity. I also renamed currentState to state for better alignment with react and vue wrappers.

index.d.ts Outdated
type Send<Context, Schema, Events extends EventObject> = Interpreter<Context, Schema, Events>['send'];

type InterpretedUsable<Context, Schema, Events extends EventObject> = {
currentState: State<Context, Events>;
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be updated as well

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented May 24, 2020

Just tried with embroider, and am currently getting this error:

Build Error (WaitForTrees)

unable to resolve package ember-resource from /tmp/.../ember-statecharts
ERROR Summary:

  - broccoliBuilderErrorStack: Error: unable to resolve package ember-resource from /tmp/embroider/dc7fab/node_modules/ember-statecharts
    at MovedPackageCache.resolve (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/package-cache.js:30:21)
    at /home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/package.js:164:38
    at Array.map (<anonymous>:null:null)
    at Package.get dependencies (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/package.js:157:22)
    at Package.<anonymous> (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/typescript-memoize/dist/memoize-decorator.js:67:52)
    at CompatAddons.linkNonCopiedDeps (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/compat/src/compat-addons.js:186:29)
    at /home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/compat/src/compat-addons.js:106:22
    at Array.forEach (<anonymous>:null:null)
    at CompatAddons.build (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/compat/src/compat-addons.js:70:48)
    at WaitForTrees.build (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/wait-for-trees.js:60:21)
    at TransformNodeWrapper.build (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/node_modules/broccoli/dist/wrappers/transform-node.js:67:39)
    at /home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/node_modules/broccoli/dist/builder.js:118:30
    at runMicrotasks (<anonymous>:null:null)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at Builder.build (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/node_modules/broccoli/dist/builder.js:137:13)
    at Builder.build (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/lib/models/builder.js:197:11)

  - code: [undefined]
  - codeFrame: unable to resolve package ember-resource from /tmp/embroider/dc7fab/node_modules/ember-statecharts
  - errorMessage: unable to resolve package ember-resource from /tmp/embroider/dc7fab/node_modules/ember-statecharts
        at WaitForTrees (@embroider/compat/addons)
-~- created here: -~-
    at new Plugin (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/node_modules/broccoli-plugin/dist/index.js:45:31)
    at new WaitForTrees (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/wait-for-trees.js:30:9)
    at CompatAddons.get tree [as tree] (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/compat/src/compat-addons.js:47:16)
    at CompatApp.augment (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/build-stage.js:57:64)
    at CompatApp.get tree (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/build-stage.js:26:50)
    at CompatApp.<anonymous> (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/typescript-memoize/dist/memoize-decorator.js:67:52)
    at new PackagerRunner (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/core/src/to-broccoli-plugin.js:10:26)
    at defaultPipeline (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/@embroider/compat/src/default-pipeline.js:35:12)
    at module.exports (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/ember-cli-build.js:125:12)
    at Builder.readBuildFile (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/lib/models/builder.js:49:14)
    at Builder.setupBroccoliBuilder (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/lib/models/builder.js:63:22)
    at new Builder (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/lib/models/builder.js:29:10)
    at ServeTask.run (/home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/lib/tasks/serve.js:49:7)
    at /home/preston/Development/NullVoxpopuli/emberclear/packages/frontend/node_modules/ember-cli/lib/models/command.js:238:24
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

Still investigating, but want to document here as it may be relevant for someone in the future.

@LevelbossMike
Copy link
Owner Author

LevelbossMike commented Jun 5, 2020

@pangratz @NullVoxPopuli can you please recheck the PR? - getting ready to release useMachine 🎉

@LevelbossMike LevelbossMike changed the title [WIP][Beta-release] useMachine [Beta-release] useMachine Jun 5, 2020
Copy link
Collaborator

@pangratz pangratz left a comment

Choose a reason for hiding this comment

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

BEAUTIFUL 🚢 🇮🇹

@LevelbossMike LevelbossMike merged commit 3aa997d into master Jun 10, 2020
@LevelbossMike LevelbossMike deleted the use-machine branch June 10, 2020 12:16
@LevelbossMike
Copy link
Owner Author

@NullVoxPopuli @pangratz This is now in and released as 0.10.0. Thanks for participating and all the help from both of you 👏

@pangratz
Copy link
Collaborator

YAPADAPADUUUUUU

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.

[SPIKE needed] Ergonomic Improvement Ideas
3 participants