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

[v5] Rename machine.withConfig(...) to machine.provide(...) #1808

Merged
merged 8 commits into from
Jan 18, 2021

Conversation

davidkpiano
Copy link
Member

Reference: https://github.com/davidkpiano/xstate/projects/1#card-40598347

This PR also renames options.behaviors to options.actors, as previously discussed.

@changeset-bot
Copy link

changeset-bot bot commented Jan 3, 2021

🦋 Changeset detected

Latest commit: a6acd40

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

This PR includes changesets to release 11 packages
Name Type
xstate Major
@xstate/graph Major
@xstate/immer Major
@xstate/inspect Major
@xstate/react 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

@davidkpiano davidkpiano changed the title Rename machine.withConfig -> machine.withOptions, options.behaviors -> options.actors [v5] Rename machine.withConfig(...) to machine.withOptions(...) Jan 3, 2021
@@ -18,7 +18,7 @@ const machine = createMachine(
},
{
- activities: {
+ behaviors: {
+ actors: {
Copy link
Member

Choose a reason for hiding this comment

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

a good change 👍 you still mention behaviors in a lot of places, I wonder if we want to stick with that terminology? I see it as potentially very confusing for people, at the very least maybe we should bury it deep down and only mention it in like "the very advanced" section of the docs? 😅

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

I'm not suuuuper-stoked about this change but on the other hand, both versions are so generic that it's hard to assess which one is better. Since we provide lookups for actions/actors/guards/etc in this, maybe it would be worth renaming this to withImplementations? It's somewhat mouthful though.

Don't take me wrong - I don't really mind the change, so do whatever you think is best for this one. The @johnyanarella's input could be valuable here as well.

@davidkpiano
Copy link
Member Author

I'm not suuuuper-stoked about this change but on the other hand, both versions are so generic that it's hard to assess which one is better. Since we provide lookups for actions/actors/guards/etc in this, maybe it would be worth renaming this to withImplementations? It's somewhat mouthful though.

Don't take me wrong - I don't really mind the change, so do whatever you think is best for this one. The @johnyanarella's input could be valuable here as well.

It's important for consistency in terminology, and was mentioned externally which is why I made a note of it:

const machine = createMachine(config, options);
//        ______________________⬆        ⬆
//        |                               |
machine.withConfig(/* ... */) //          |
//        ________________________________|
//        |
machine.withOptions(/* ... */)

@Andarist
Copy link
Member

Right - the consistency is definitely an improvement but it could be approached from the other end (I'm not saying that it should be). You could rename the createMachine parameters to smth like:

createMachine(schema, implementations)

I didn't even realize that those 2 were currently named like this and it seems to me that it can still be quite confusing because, as mentioned, both config & options are quite generic so if I say "Change your config" one still can be confused on which thing they should adjust.

Not a big issue though - naming is hard 🤷‍♂️ I don't really have any strong contenders for the chosen names.

@davidkpiano
Copy link
Member Author

Implementation is the "correct" answer but you're right, it's not the most dev-friendly word to type out 🤔

@Silverwolf90
Copy link
Contributor

Apologies, but I couldn't resist some drive-by bikeshedding... but if open to dropping withXXX convention, what about use (or using)

machine.use({ ... })
machine.using({ ... })

anyway, sorry again, feel free to ignore 😄

@davidkpiano
Copy link
Member Author

davidkpiano commented Jan 15, 2021

@Silverwolf90 Thanks for the suggestion - still bikeshedding this!

What do you think about:

-machine.withConfig(/* ... */)
+machine.provide(/* ... */)

As in, "provide these implementations"?

@johnyanarella
Copy link

I think it will be worth all the bikeshedding in the end - I'm excited about the potential that tweaking the names here has for reducing the learning curve for newcomers.

Moving away from "configuration" is great news, given SCXML already uses it in this problem space—adhering to the classical definition of an arrangement or combination of things—to describe the set of currently active states. The interpreter "options" conform to the common expectation of a fixed set of knobs and toggles with defaults. Using that same term to describe passing a machine an arbitrary and unbounded set of methods doesn't feel like a good fit.

At one point I think I halfheartedly suggested the idea of calling the thing passed in here a "wiring harness". Not keen on that one, but I do still wonder if we could mine the terminology associated with machines in the physical world to find something familiar (and less abstract) that intuitively and unambiguously conveys the idea here.

Determining the right terminology to describe this pattern—referencing actions/actors/guards by name and plugging in language native implementations—will make a huge difference in highlighting this capability, its purpose and value, as the XState community expands beyond JavaScript.

I gotta concede that withImplementations() is precise, but a real pain to type. provide() is compelling. That method name and behavior will be familiar to anyone who has used an IoC container API, which is essentially what's happening here...

@Andarist
Copy link
Member

"options" conform to the common expectation of a fixed set of knobs and toggles with defaults

This might be the best description for options that I have ever seen! Gonna probably think about it in those exact terms since now - I would only say that not all options have to provide defaults, your statement kinda reads as if that would be a requirement.

Do you have any good definition for config? 😉

@johnyanarella
Copy link

johnyanarella commented Jan 16, 2021

Fair point!

You're absolutely right about an option choice among options sometimes not even being optional ("I'll give you two options...!"). And then there's the perspective on optionality where nil is one of the options—and the default one, at that. (Or in JavaScript, the nullish undefined or null 🤣).

In practice, we see configuration, preferences, properties and settings used interchangeably in software.

Configuration is arguably distinct among those as precisely describing a specific arrangement or combination among possibilities. When you configure your MacBook Pro M1 for purchase, you pick a configuration of cpu speed, ram and ssd size options (another example where the options aren't really optional!).

Given their mathematical underpinnings, it's not surprising that the SCXML specification and even David Harel's original statecharts paper use configuration when referring to a distinct set of states among possible states. Harel is even more precise in referring to it as a "state-configuration".

Anyway:

  • I still think the option (😮) of using config for anything other than a state configuration here has been precluded by the pre-existing use of configuration in this particular problem space; and
  • provide() makes more and more sense the longer I've had to think about it, as this is performing DI, and that's a pretty conventional API for one; and
  • I'm not sure what you name the type of the thing you would pass to provide().

@johnyanarella
Copy link

johnyanarella commented Jan 16, 2021

Regarding this suggestion above:

createMachine(schema, implementations)

Over in xstate-swift, I currently have:

// an instance of MachineSchematic created either via the Swift DSL or parsed from SCXML
let machineSchematic = MachineSchematic.fromSCXML(data: scxmlData)

// an instance of MachineOptions mapping named references to native implementations
let machineOptions = MachineOptions(
    actions: ...,
    actors: ...,
    guards: ...
)

let machine = Machine(schematic: machineSchematic, options: machineOptions)

The term schema comes with almost as much baggage as state.

I gravitated toward MachineSchematic and schematic there as it has some familiar association with the term Machine and helps convey their relationship—the schematic is inert on its own and is a blueprint used later to construct a machine.

As we've discussed above, I'm no longer convinced MachineOptions or options is ideal here. implementations is compelling, but mouthful, and MachineImplementations leaves a lot to be desired as a type name.

@davidkpiano
Copy link
Member Author

@johnyanarella Super helpful thoughts, thanks. So just to digest this, the createMachine() factory takes two arguments:

  1. The machine schema, which is its partial definition
  2. The implementation details provided to the machine

For 1., the full definition can be provided as well, and providing a partial definition is just for improved DX, since manually typing out a full definition is tedious, and a lot of things can be inferred (I'm saying this to justify definition as a term).

So, for tentative naming, I propose this:

createMachine(definition, implementations);

And for the original PR, this:

someMachine.provide(implementations);

Of course, the developers would not need to type out "implementations" - that would just be what's present in the documentation and types.

@Andarist
Copy link
Member

I very much like where this bike-shedding has brought us to 👍

one important realization is that with this change we effectively seal what the second parameter is used for. We should be cautious in the future not to introduce there anything that is not an „implementation”

@johnyanarella
Copy link

Nice!

I'll mirror this terminology choice over into xstate-swift. Everything there that comprises the MachineDefinition (resulting from parsing SCXML or the Swift DSL) is named with a similar suffix already (StateDefinition, TransitionDefinition, etc.), so that lines up nicely.

With Swift, I've got to contend with labelled parameters. So, I'll probably sidestep implementations in externally facing APIs where I can with a provided label:

let machine = Machine(
    definition: MachineDefinition.fromSCXML(data: scxmlData),
    provided: ImplementationDetails(
        actions: [
            "foo": { context, event, meta in
              // ...
            }
        ],
        // ...
    )
)

(barring anyone coming up with a better type name for ImplementationDetails).

@johnyanarella
Copy link

...or did I misinterpret your comment above, and MachineSchema is the type name for your definition parameter?

@johnyanarella
Copy link

(Worth noting, that the same possibility exists in xstate-swift of passing a full definition and omitting the second parameter, if you've 1) used the Swift DSL and don't reference actions/actors/guards/etc. by name; or 2) parsed an SCXML document that needs no native implementation details.)

@davidkpiano davidkpiano changed the title [v5] Rename machine.withConfig(...) to machine.withOptions(...) [v5] Rename machine.withConfig(...) to machine.provide(...) Jan 17, 2021
@davidkpiano davidkpiano merged commit 0497da9 into next Jan 18, 2021
@davidkpiano davidkpiano deleted the v5/config-to-options branch January 18, 2021 16:14
@devanshj
Copy link
Contributor

So, for tentative naming, I propose this:

createMachine(definition, implementations);

Stumbled upon this, any chance txstate influenced this naming or just a coincidence? :P

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.

5 participants