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

Added entry & exit function support #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rkoshy
Copy link

@rkoshy rkoshy commented Apr 20, 2020

The changes allow each state to have an _entry and an _exit handler that acts as a special transition that executes on entry into, or exit from a state.

The changes allow each state to have an `_entry` and an `_exit` handler that acts as a special transition that executes on entry into, or exit from a state.
Copy link
Owner

@krasimir krasimir left a comment

Choose a reason for hiding this comment

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

Looks like a decent idea.

I'm fine dealing with the last bit but can you add a test coverage of the changes.

@@ -24,10 +25,23 @@ export default function updateState(machine, state) {
throw new Error(ERROR_UNCOVERED_STATE(newState.name));
}

var handler = machine.transitions[machine.state.name]['_exit'];
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use @exit and @enter instead of _exit and _entry. It seems more intuitive for me. What you think?

Copy link
Author

Choose a reason for hiding this comment

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

I am not tied to any specific terminology. We could use @exit and @enter - but I see the @ symbol potentially being potentially problematic. The _ was more based on my understanding of the convention being used in JS to represent 'private' members/methods.

@illarionvk
Copy link
Contributor

illarionvk commented Apr 22, 2020

I like the @enter/@exit naming schema.

Would it also make sense to omit those internal-use actions from the final machine methods, i.e., the machine should not have machine.enter() and machine.exit() methods?

@krasimir
Copy link
Owner

@illarionvk what are your concerns about having those defined. They'll be actually @enter and @exit.

@illarionvk
Copy link
Contributor

The camelcasing transform would rename the methods to .enter() and .exit(), wouldn't it?

I think having literal non-camelcased machine['@enter'] methods is ok, as long as the developer can define their own, explicit, enter/exit methods too:

const noop = function() {}

const config = {
  state: IDLE,
  transitions: {
    [IDLE]: {
      "@exit": function() { ... },
      init: function() { ... }
    },
    [INITIALIZING]: {
      "@enter": function() { ... },
      noop
    },
    [READY_TO_PROCESS]: {
      enter: function() { ... }
    },
    [PROCESSING]: {
      exit: function() { ... }
    },
    [DONE]: {
      noop
    }
  }
};

// The config should produce an instance similar to the type below
type MachineInstance {
  ...Machine,
  '@enter': () => void,
  '@exit': () => void,
  'enter': () => void,
  'exit': () => void,
  'init': () => void,
  'noop': () => void,
}

@rkoshy
Copy link
Author

rkoshy commented Apr 22, 2020

I'm fine dealing with the last bit but can you add a test coverage of the changes.

Yes @krasimir I will add the tests.

@krasimir
Copy link
Owner

@illarionvk is completely right. We have to think about how the API will look like. The helper here will indeed remove the @, which is not necessary a bad thing but it shouldn't be exposed the same way the other actions are. I'll need to think it through. @rkoshy I'll try finding some time to explore more your PR.

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.

3 participants