statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
27.08k stars 1.24k forks source link

Complete context rewrite, not assign #989

Closed nikogosovd closed 3 years ago

nikogosovd commented 4 years ago

The currently existing assign action cannot delete properties of the context object. It only assigns new properties or changes existing ones. Is there a way in xstate to assign a completely new object to the context in action?

If not, I see this functionality as a new function e.g. replace with the API similar to the existing assign function. It takes the context "replacer", which represents how the current context should look. The "replacer" can be an object or a function and the main difference is that the context is completely replaced with the new object.

davidkpiano commented 4 years ago

EDIT: this might not be the case. Looking into this.

Yes, you can change the entire context (replace it) by using a function, like this:

// assume you have { one: 1, two: 2 }

actions: assign(ctx => {
  return { one: ctx.one }; // delete two
});

See here: https://xstate.js.org/docs/guides/context.html#assign-action

nikogosovd commented 4 years ago

Thank you for the immediate reply! Actually I tried the mentioned approach and it doesn't work as you described. Here is the reproduction: https://codesandbox.io/s/vigorous-volhard-f0en1

Am I doing something wrong?

davidkpiano commented 4 years ago

Would it be sufficient to just set { unwantedProp: undefined }?

nikogosovd commented 4 years ago

Actually not, because when I’m saving a context with undefined props in MongoDB, it replaces them with nulls. Yes, there is an option in mongo driver that prevents this behaviour, but MongoDB drivers must not be responsible for xstate’s unexpected behaviour.

With all respect to the effort spent on xstate and appreciation for the hard work of supporting and improving the code, the fact that xstate doesn’t work as you, the creator of xstate, described means that the issue requires a fix, not a workaround solution.

Andarist commented 4 years ago

I agree that I would expect the same result as @nikogosovd suggests (and which @davidkpiano also thought that it would be the case), but I'm not sure if everyone would expect it the same way. Given the fact that this logic is already in place for quite some time already, I would consider changing it right now unsafe because it could break somebody badly.

That being said - I believe that this could be tackled in upcoming v5, we only would need to check how it relates to SCXML spec. From the top of my head, it seems that there is no way in it to get "rid off" some datamodel value beyond doing explicitly what @davidkpiano has suggested - assigning undefined to all other properties.

the fact that xstate doesn’t work as you, the creator of xstate, described means that the issue requires a fix, not a workaround solution.

This is not necessarily true. We are all humans, we have faulty memory etc. It really is no surprise that something is working differently than its creator thinks it does. Even given such situation, it doesnt immediately mean that the code should be changed in a way that the author has thought it works already. Each situation is different and has to be evaluated on per-case basis. The author might have forgotten some reasoning he has made in the past, but it doesnt invalidate that reasoning - it could have been sound. I'm not saying anything here about this particular case, it's just a general remark. I bet you have also written software which has behaved in a different way that you have though it does behave.

davidkpiano commented 4 years ago

Actually not, because when I’m saving a context with undefined props in MongoDB, it replaces them with nulls. Yes, there is an option in mongo driver that prevents this behaviour, but MongoDB drivers must not be responsible for xstate’s unexpected behaviour.

This has a straightforward workaround: don't link XState's behavior directly to MongoDB, and instead introduce some sort of adapter layer:

function removeUndefinedProps(object) {
  return Object.keys(object).reduce((acc, key) => {
    if (object[key] === undefined) return acc;

    acc[key] = object[key];

    return acc;
  }, {});
}

// anywhere you write to MongoDB, use the function
function writeToMongo(data) {
  return someMongoClient.write(removeUndefinedProps(data));
}

Alternatively, you can have a property in context that represents the data to send to MongoDB, and assign({ thatProp: () => {...} }) will work as expected (regarding removed properties) since it doesn't deep-merge.

nikogosovd commented 4 years ago

@Andarist You are right, this fix can break someone's code. That's why an introduction of the replace function as I described in the first message here will not break anything. And then, the upcoming v5 may merge this functionality into the assign((ctx, event) => ({/* ... */})) like calls and remove the replace function. What do you think?

@davidkpiano thank you for the detailed workaround description, I will definitely do something like that for now before some solution in xstate is available.

the fact that xstate doesn’t work as you, the creator of xstate, described means that the issue requires a fix, not a workaround solution.

It was a kind of overreacting from my side, I apologize. Of course, I've also made mistakes. Actually I'm very happy with xstate and until now all the problems that I've faced were already solved in closed issues or in the documentation. This is an indicator of a high-quality product that you @davidkpiano and @Andarist and other contributors made.

davidkpiano commented 4 years ago

Greatly appreciated, @nikogosovd. I will add a test in V5 to ensure the assign replace behavior is exactly as you expect it to be here.

Andarist commented 4 years ago

I have also thought that this change would be sound and good at first, but now I'm questioning it a little bit.

  1. It deviates from the SCXML and makes the conversion harder
  2. It hides implicit behavior - which is "remove other keys", but which keys are those? what if we add more context keys in the future? with the proposed change they would be removed as well, potentially introducing an easy to oversight bug. The current behavior seems safer in this regard - you have to opt-in to "remove" explicit keys. I understand that this might cause you, @nikogosovd, a little bit of a headache because lack of the key and undefined value are not the same, but the proposed by @davidkpiano solution with removeUndefinedProps is completely fine. It's perfectly understandable, that you might need to do some extra conversion when integrating with some other system (Mongo in this case)
davidkpiano commented 3 years ago

Going to label this as "working as designed" - it doesn't make sense to delete properties from context directly, as it should maintain the same shape throughout the lifetime of the machine. If you want to represent an object in context that has properties that can be deleted, that object should itself be a property of context:

createMachine({
  context: {
    objectWithDeletableProperties: {/* ... */}
  },
  // ...
});
p-j commented 3 years ago

Hi

Sorry for "reopening" @davidkpiano but while using Typescript I'm having trouble assigning undefined

I have tried:

unassignTour: assign(() => { tour: undefined })
unassignTour: assign({ tour: () => undefined })
unassignTour: assign({ tour: undefined })

The first two giving me the following error:

Type 'AssignAction<{ tour: any; }, ContainerEvent>' is not assignable to type 'ActionObject<ContainerContext, ContainerEvent> | ActionFunction<ContainerContext, ContainerEvent>'.
  Type 'AssignAction<{ tour: any; }, ContainerEvent>' is not assignable to type 'ActionObject<ContainerContext, ContainerEvent>'.
    Types of property 'exec' are incompatible.
      Type 'ActionFunction<{ tour: any; }, ContainerEvent>' is not assignable to type 'ActionFunction<ContainerContext, ContainerEvent>'.
        Types of parameters 'context' and 'context' are incompatible.
          Type 'ContainerContext' is not assignable to type '{ tour: any; }'.
            Property 'tour' is optional in type 'ContainerContext' but required in type '{ tour: any; }'.

While the last one is giving me

Type 'AssignAction<{ tour: undefined; }, ContainerEvent>' is not assignable to type 'ActionObject<ContainerContext, ContainerEvent> | ActionFunction<ContainerContext, ContainerEvent>'.
  Type 'AssignAction<{ tour: undefined; }, ContainerEvent>' is not assignable to type 'ActionObject<ContainerContext, ContainerEvent>'.
    Types of property 'exec' are incompatible.
      Type 'ActionFunction<{ tour: undefined; }, ContainerEvent>' is not assignable to type 'ActionFunction<ContainerContext, ContainerEvent>'.
        Types of parameters 'context' and 'context' are incompatible.
          Type 'ContainerContext' is not assignable to type '{ tour: undefined; }'.
            Types of property 'tour' are incompatible.
              Type 'Tour' is not assignable to type 'undefined'.

With the ContainerContext containing a tour?: Tour prop

If I make the prop tour: Tour | null and use unassignTour: assign({ tour: () => null }) it does work but that's not exactly what I want. Note that it won't work if I set the prop to tour?: Tour | null the fact that the prop is optional seems to be the root of the issue here.

Is there a workaround for that or am I stuck initializing and carrying null values? Thanks

davidkpiano commented 3 years ago

Is there a workaround for that or am I stuck initializing and carrying null values?

Yes:

assign<ContainerContext>({ tour: undefined })

When inference fails, be explicit.

andrejpavlovic commented 1 year ago

In order to futureproof the logic of resetting all properties in the context in a type-safe way, I've used satisfies to ensure no properties are left unassigned when resetting the context.

const service = useInterpret(baseMachine, {
  context: {},
  actions: {
    reset: assign({
      prop1: undefined,
      prop2: undefined,
    // ensure all context keys are reset
    } satisfies {[K in keyof Required<ContextFrom<typeof baseMachine>>]: ContextFrom<typeof baseMachine>[K]})
  },
})

If a new property, e.g. prop3, was added to the context in the future, the code about would show a type error, requiring prop3 to be added.

Andarist commented 1 year ago

That's not a bad solution at all! I like it :)