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

[SPIKE needed] Ergonomic Improvement Ideas #235

Closed
LevelbossMike opened this issue Apr 21, 2020 · 10 comments · Fixed by #241
Closed

[SPIKE needed] Ergonomic Improvement Ideas #235

LevelbossMike opened this issue Apr 21, 2020 · 10 comments · Fixed by #241
Assignees

Comments

@LevelbossMike
Copy link
Owner

LevelbossMike commented Apr 21, 2020

As discussed with @NullVoxPopuli, @pangratz and @sukima in a recent call we want to try to improve the ergonomics of the addon for modern Ember.js usage.

Areas for improvement:

  • make it easy and intuitive to use all features from xstate in an Ember.js application
  • allow passing machines directly and not necessarily create an interpreter via the statechart-decorator.
  • don't hide xstate behind custom addon related "magic"

We came up with some ideas about a new way to integrate xstate with ember-statecharts:

Using a mechanism similiar to @use and Resources- RFC

import Component from '@glimmer/component';
import { useMachine, matchesState } from 'ember-statecharts';
// to extract machine configs to a different place than actual component code
import TestMachine from '../machines/test';


export default class Test extends Component {
  /*
   @use does not exist yet. We could imagine building something similar
   ourselves for now to emulate its behavior for now - maybe we could call
   it @useMachine
  */
  @use statechart = useMachine(TestMachine)
    .withConfig({ /* ... */}) // to allow calling component to customize `TestMachine`-machine
    .withContext({/*...*/}); // to allow calling component to customize `TestMachine`-machine

  // still need tbd if we want to keep @matchesState
  @matchesState('testing')
  isTesting: boolean;
}

Benefits of this approach

  • aligned with the future vision of the framework, i.e. @use
  • we can hook into the component lifecycle to start the underlying xstate-interpreter and clean it up when the component gets unrendered / destroyed
  • it looks very similar to what other framework wrappers of xstate do - e.g. xstate/react or xstate/vue

Questions

  • how can we emulate the @use-decorator?
  • does it make sense to write an @use-like functionality just for this addon that we can later on switch out for @use?

Next steps

  • Do a spike implementation to see if it is possible to implement this api with the current primitives the language and framework provide.

@NullVoxPopuli, @pangratz, @sukima as far as I remember we were all pretty much in favor of this approach so I'm adding the other versions we discussed only for reference.

Other ideas:

class decorator

import Component from '@glimmer/component';
import TestMachine from '../machines/test';

@useMachine(TestMachine)
export default class Test extends Component {
  @withConfig
  get() {
    return {
      actions: {
        handleSuccess: this.handleSuccess
      }
    }
  }

  @withContext
  get() {
    return {
      count: 1
    }
  }

  @statechartAction
  handleSuccess(context: any, event: { type: string, ...}) {
    // context.count => 1
  }

  @transition
  inputChanged(send: () => {}, event: { target: { value }}) {
    send('INPUT_CHANGED', { value });
  }
}

Property decorator

import Component from '@glimmer/component';
import TestMachine from '../machines/test';

export default class Test extends Component {
  @useMachine(TestMachine)
  interpreter;

  @withConfig
  get() {
    return {
      actions: {
        handleSuccess: this.handleSuccess
      }
    }
  }

  @withContext
  get() {
    return {
      count: 1
    }
  }

  @statechartAction
  handleSuccess(context: any, event: { type: string, ...}) {
    // context.count => 1
  }

  @action
  inputChanged(event: { target: { value } }) {
    this.interpreter.send('INPUT', { value });
  }
}
@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Apr 23, 2020

In addition, we should explore different ways of handling ember-concurrency tasks as machine-invokables.

@sukima has a wonderful little utility here that enables tasks to be located anywhere outside the lifecycle of the machine: https://tritarget.org/#ember-concurrency%20with%20XState:[[ember-concurrency%20with%20XState]]%20[[Ember%20XState%20Utility]]
(which is handy for my use case, where I'm defining tasks on ephemeral classes that aren't exactly tied to component-state (though that's easier now with the destroyables polyfill / RFC).

It may be beneficial to support the scenario where a nested component contains a task that the machine is aware of and something cancels the task. Supporting both cancellation and error would allow for slightly different workflows, if needed.


@sukima's Code copied here

import { didCancel } from 'ember-concurrency';

/**
 * Wraps an ember-concurrency task into an XState service.
 *
 *     Machine({
 *       id: 'example',
 *       initial: 'fetch',
 *       states: {
 *         'fetch': {
 *           invoke: { src: 'fetch' },
 *           on: {
 *             DONE: 'done',
 *             CANCEL: 'cancelled',
 *             ERROR: 'errored',
 *           },
 *         },
 *         'cancelled': { type: 'final' },
 *         'errored': { type: 'final' },
 *         'done': { type: 'final' },
 *       },
 *     }).withConfig({
 *       services: {
 *         fetch: taskService(this.fetchTask),
 *       },
 *     });
 *
 * @function
 * @param {TaskProp} taskProp the task property (not instance) to call perform() on
 * @return {CallbackService} an XState compatable callback based service
 */
export function taskService(taskProp) {
  return (...args) => (callback) => {
    let taskInstance = taskProp.perform(...args);
    taskInstance.then(
      (data) => callback({ type: 'DONE', data }),
      (error) => didCancel(error)
        ? callback({ type: 'CANCEL' })
        : callback({ type: 'ERROR', error })
    );
    return () => taskInstance.cancel();
  };
}

@LevelbossMike
Copy link
Owner Author

@NullVoxPopuli
Copy link
Contributor

cc @pzuraq
VERY EXCITE

@pzuraq
Copy link

pzuraq commented Apr 26, 2020

hmm, I need to finish that, this branch is where I'm taking it but haven't had time to finish it up and land it.

Seems like a perfect use case for @use though 😄

@alexlafroscia
Copy link

alexlafroscia commented Apr 27, 2020

I’m curious about the syntax like

  @matchesState(‘testing’)
  IsTesting: Boolean

Reading something like that throws me off; it reminds me of something like that the eq decorator and friends, but there’s no explicit pointer to the state machine that’s in the testing state. I guess it assumes that the chart itself is available as the statechart property, but does it make sense for the API to make that assumption? To me at least, it makes things harder to read as I end up asking myself “what matches this state?”

@NullVoxPopuli
Copy link
Contributor

but there’s no explicit pointer do the state machine

yeah, that's my feeling about it too. And then what happens if someone wants 2 or more state machines in a component? those decorators wouldn't be able to work.

I think exporting some vanilla JS helpers will help a bunch, as well as some helpers to use in the template. I think there should be some explicitness about which machine is being used where. idk.
I'm personally uncomfortable with forcing a single machine to one component

@alexlafroscia
Copy link

happens if someone wants 2 or more state machines in a component? those decorators wouldn't be able to work.

That’s a second concern of mine as well that goes right along with my confusion. I’ll admit that I know nothing about xstate so maybe “one component manipulating two state charts” is just bad design, but I have a hunch that’s not something that should be totally off-the-books. If a redesign of the API for the library is in discussion, I’d love to see this concern explored! Even just expanding the API to something more like

@matchesState(‘statechart’, ‘testing’)
IsTesting

Would be much easier to grok on a quick read without context (which will be a lot of people reading most code!)

@LevelbossMike
Copy link
Owner Author

LevelbossMike commented Apr 27, 2020

I’m curious about the syntax like

  @matchesState(‘testing’)
  IsTesting: Boolean

Reading something like that throws me off; it reminds me of something like that the eq decorator and friends, but there’s no explicit pointer to the state machine that’s in the testing state. I guess it assumes that the chart itself is available as the statechart property, but does it make sense for the API to make that assumption? To me at least, it makes things harder to read as I end up asking myself “what matches this state?”

The idea was that you usually only have one so it's easier to just have a default property that it matches against which safes some typing. Right now the default is statechart. But it already works for a different property name even if you had two statecharts in your component:

https://github.com/LevelbossMike/ember-statecharts/blob/master/tests/unit/computed-test.js#L101

matchesState('started', 'testingStatechart')

Will match against the testingStatechart-interpreter - the @statechart-decorator just returns an xstate interpreted machine - something xstate refers to as service.

It might make sense to have configuration option for this though. So if you call you statechart properties different you might want to change the default in one place instead of always doing @matchesState(<state>, 'myCustomStatechartPropertyNameIUseEverywhere').

If you don't like this implicitness you can always do @matchesStates(<state>, 'statechart') already.

I totally understand the magical feel though and that it trips people up at first. It might makes sense to change the docs to the explicit version - because many people share this feeling when getting into ember-statecharts for the first time. 🤔

@alexlafroscia
Copy link

If you don't like this implicitness you can always do @matchesStates(, 'statechart') already.

That’s awesome!

It might makes sense to change the docs to the explicit version - because many people share this feeling when getting into ember-statecharts for the first time. 🤔

That makes a lot of sense to me! Sorry to derail the conversation from the initial purpose, but I thought that addressing that feedback might be tied up with addressing the API of the addon as a whole.

@sukima
Copy link

sukima commented Apr 27, 2020

The idea of these decorators is to wrap the boilerplate up in a neat tidy package. Thus such I would argue one component running two or more machines is out of scope and to implement that yourself. The boilerplate to interface with XState is so minimal it is very easy to use it out-of-the-box. That said one could also just have two EmberObjects each with their own @useMachine and one component that manages them.

There are so many ways to skin the cat there does not need to be one twue way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants