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

Freshness broken for empty state #13423

Closed
basarat opened this issue Jan 11, 2017 · 9 comments
Closed

Freshness broken for empty state #13423

basarat opened this issue Jan 11, 2017 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@basarat
Copy link
Contributor

basarat commented Jan 11, 2017

TypeScript Version: nightly

Code

class Foo extends React.Component<{}, {}>{
  render() { return <noscript /> }
  foo() {
    this.setState({ something: true }); // Should error but doesn't
  }
}

🌹

@normalser
Copy link

normalser commented Jan 11, 2017

Isn't it because you can assign anything to {} ?

let a:{} = {something:true} // OK

FYI https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-all-types-assignable-to-empty-interfaces

@basarat
Copy link
Contributor Author

basarat commented Jan 11, 2017

It is and thats the issue I should have created.

But is there a reason why {} should be allowed to bypass the strict object checking (freshness) rule ? ❤️

@basarat
Copy link
Contributor Author

basarat commented Jan 11, 2017

@wallverb Thanks!

In general, you should never find yourself declaring an interface with no properties.

But we do cause setState is dangerous for beginners https://medium.com/@mweststrate/3-reasons-why-i-stopped-using-react-setstate-ab73fc67a42e#.ja8gt8txu so we swapped state to {}

And then migrating code from setState to @observable becomes painful to clean up as all those calls to setState don't highlight as errors. We instead needed to painfully find references to setState 🌹

@normalser
Copy link

normalser commented Jan 11, 2017

@basarat Maybe exact types would help #12936

Here is the result in Flow:

type Foo = {  };
type Bar = {|  |};

const foo:Foo = { a: 4 } // Works
const bar:Bar = { a: 5 } // Error Property `a` not found in object type

or use never as State type in your React component

@basarat
Copy link
Contributor Author

basarat commented Jan 11, 2017

They would help.

I would still argue that it is okay that {foo:true} works with {}, but should respect the freshness check just like other things that fail in the presence of freshness 🌹

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jan 12, 2017

{ } is a frequent "safer" alternative to any for input positions.

Freshness detection is generally intended to detect typos (this.setState({ weigt: 12 });); the "typo" of including a property where none are allowed (?) is a very strange error to make. It's very unlikely the recipient of the value demands an empty object -- why would anyone be doing that?

@basarat
Copy link
Contributor Author

basarat commented Jan 12, 2017

why would anyone be doing that

@RyanCavanaugh for migrating off of setState : #13423 (comment) 🌹

@RyanCavanaugh
Copy link
Member

If you want something that nothing is assignable to, use void

@basarat
Copy link
Contributor Author

basarat commented Jan 12, 2017

If you want something that nothing is assignable to, use void

Excellent. That solves my need. Closing as I don't need this changed anymore 🌹 and the decision to do the freshness checks for {} is a choice I leave for you / other humans 🤗 , create an issue or ignore ❤️

@basarat basarat closed this as completed Jan 12, 2017
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jan 12, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants