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

Writeonly<S> as a solution for the problem with React's setState() and strictNullChecks #27521

Closed
4 tasks done
JannesMeyer opened this issue Oct 3, 2018 · 9 comments
Closed
4 tasks done
Labels
Duplicate An existing issue was already created

Comments

@JannesMeyer
Copy link

JannesMeyer commented Oct 3, 2018

Search Terms

  • setState

I found these similar issues:
#12793 (closed without a proper solution)
#13359 (quite a different use case/problem + no solution offered)

Suggestion

My suggestion would be a new special generic Writeonly<S> (the opposite of Readonly<S>).

Like that you could write code like this:

interface S {
    foo: number;
}

class Test extends React.Component<{}, S> {

    handleClick = () => {
        let s: Writeonly<S> = {}; // Works! With Pick<S, 'foo'> this would not be allowed

        // Protects against the following bug:
        //console.log(s.foo + 1);

        s.foo = 1;

        // Protects against the following bug. With Partial<S> this would be allowed:
        //s.foo = undefined;

        this.setState(s); // Works! With Partial<S> this would not work, because it can't assign undefined to number
    };

}

Use Cases

Using React's setState() in conjunction with strictNullChecks, which still doesn't have a proper solution. Neither Pick<S, T> nor Partial<S> are very useful when strictNullChecks is enabled. Especially when you need to update properties in an if-branch.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@JannesMeyer JannesMeyer changed the title Writeonly<S> as a solution for the problem with React.setState() and strictNullChecks Writeonly<S> as a solution for the problem with React's setState() and strictNullChecks Oct 3, 2018
@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 3, 2018

You have checked the third point of the checklist, but how would this work without emitting different JS? Your property s.foo has the value undefined now, so setState will take that value and assign it to your state (which is not legal according to the types).

In order for this to work as you imagine it you would need to actually remove the property from the object at run-time again, so instead of just type information you need additional JavaScript that would change

s.foo = undefined;
s.bar = methodMayReturnUndefined();

to

delete s.foo;
s.bar = methodMayReturnUndefined();
if (s.bar === undefined)
    delete s.bar;

@JannesMeyer
Copy link
Author

JannesMeyer commented Oct 3, 2018

Sorry, that line of code was only an illustration of why Partial<S> doesn't work for this use case. I would never try to assign undefined. I thought that was obvious. Sorry about the confusion!!

I have edited the post above and converted the line into a comment.

@JannesMeyer
Copy link
Author

JannesMeyer commented Oct 3, 2018

I would like to add that today the code above can be achieved by lying about the type with as:

handleClick = () => {
   let s = {} as S;

   console.log(s.foo); // Problem: s.foo has the type number in TypeScript, but it is undefined.
   // That's why reading from Writeonly<S> should not be allowed

   s.foo = 1;
   this.setState(s);
};

But this is much less safe, because you could read s.foo and receive undefined (even though TypeScript thinks you got a number). That's why reading properties should be disallowed on Writeonly<S> objects.

PS: This additional safety might not seem very important, but keep in mind that this minimal code sample does not reflect real world applications in any way. In real applications the state objects can be quite a lot larger and the state updating logic can involve a lot of if-conditionals, etc.

@ghost
Copy link

ghost commented Oct 3, 2018

I think most people would prefer just this.setState({ foo: 1 }) over creating an empty object and writing to its properties one-by-one.
#13195 is a better solution if the problem is that you want all defined properties to be non-undefined.

@JannesMeyer
Copy link
Author

JannesMeyer commented Oct 3, 2018

Yes, that's better for simple cases indeed. But it doesn't allow you to make an assignment based on a condition.

Let's say you want to update foo and bar when a condition is true, but you always want to update a and b regardless:

if (condition) {
    this.setState({ foo: 1, bar: 2 });
}
this.setState({ a: 1, b: 2 });

However, the problem with multiple setState() calls is that these are not guaranteed to be batched by React. (Especially in async code they are never batched)

So if you want to do it in a single setState() to avoid duplicate renders you need to create an object and assign them one by one:

let state = {} as S;
if (condition) {
    state.foo = 1;
    state.bar = 2;
}
state.a = 1;
state.b = 2;
this.setState(state);

This is the only way to do it with a single setState().

And now imagine that you are doing this in a larger application with more complex state objects/logic. It would definitely be helpful to be able to communicate to other programmers on the team that this is a non-readable object.

@RyanCavanaugh
Copy link
Member

Duplicate #21759 ? Readonly is not a special type; it's just a mapped type applying the readonly property

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Oct 3, 2018
@Jessidhia
Copy link

Jessidhia commented Oct 4, 2018

@andy-ms this is usually a problem when writing setState in its callback form, which is how you're supposed to do it if the next state depends at all on the current state. (Not doing it might break your logic in AsyncMode)

The following example cannot compile at all with the current type definitions:

this.setState((state, props) => {
  if (props.foo) {
    return { bar: state.bar + 1 }
  }
  return { baz: state.baz - 1 }
})

A workaround is to copy the value you want to preserve; adding baz: state.baz to the first object and bar: state.bar to the second object. But all objects you return from the callback must currently have the exact same keyof.

@JannesMeyer
Copy link
Author

@RyanCavanaugh Thanks for pointing me to the other issue. I haven't read the entire discussion yet, so I'm not sure if that would allow the creation of an empty object like this:

let s: Writeonly<S> = {}; // S has many required keys

I will read through that issue when I have some more time.

@Kovensky Thanks, that is another great example of why something like this is needed! That situation is exactly why I created this feature request.

@typescript-bot
Copy link
Collaborator

This issue has been marked as a duplicate and has seen no activity in the last day. It has been closed for automatic house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants