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

Design Meeting Notes, 12/16/2016 #12997

Closed
DanielRosenwasser opened this issue Dec 17, 2016 · 11 comments
Closed

Design Meeting Notes, 12/16/2016 #12997

DanielRosenwasser opened this issue Dec 17, 2016 · 11 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

Intrinsic object Type (#12501)

Can't be an object-type (in terms of how the spec/compiler defines object types).
This is more like a primitive.

  • Don't want it to be assignable to any other primitive.
  • Will have an apparent type of Object.
  • Anything is assignable to {} (and almost anything is assignable to Object).
    • So object should be assignable to {} and Object.
    • What about Function?
      • Hesitance about agreement.
      • Saying "no" means you can never pass in anything with a call signature.
      • But functions are objects.
      • Does it have it in its prototype?
  • This is "cheatable" construct (i.e. number -> Number -> object).
    • Yes, but this largely an intent-based construct.
      Covers a majority of the use-cases.
  • What about people who want to ensure that you are an object with some set of optional properties?
    • People may write object & { a?, b?, c? }.
      • And subsequently type ObjectOf<T> = object & t.
      • Nobody will figure that out.
        • Well someone absolutely will, and why shouldn't they do this?
    • But implementing weak types would avoid some of the issues with this.
    • Are we implementing weak types?
      • Potentially.
        • Well then people are going to write this!
    • Kind of unfortunate that when people write { a?, b?, c? } they truly do mean they only want an object-type.
  • A lot of the ideas being discussed at this meeting have to do with weak types, exact types, etc.

Conclusion:

  • 👍 for object, still need to think about exact types.
  • Current PR needs some changes, let's work with @HerringtonDarkholme to get that done.

Excess Property Checks on Union types (#12745)

Is a pretty complex problem.

  • Currently we disable excess property checks for intersections and unions.
    • Why intersections?
      • Nobody is sure. We just did it?
        • Let's not do that.

#12904

  • This is not excess property checking, this is weak type checks.
  • Some of the conversation here has to do with the idea of flattening intersections of solely object types.
    • But makes inference problematic.
      • But it doesn't quite work with object literals anyway.
      • Still happens in other places.
    • We could create a type that knows about its originating type (i.e. a special object type that knows about its original intersection type).

Options:

  • Union type property checks

  • Weak types

  • Exact types

  • If we did exact types

    • Could you write exact interfaces?
    • How does it play with intersections?
    • Could you have an exact type parameter? What does it mean to have a generic exact type?
  • If we did weak type detection

    • A heuristic.
    • Will likely catch a good class of bugs.
    • But doesn't quite have the full power of an exact type.

Freshness Maintenance for Object Spread (#12717)

  • Is there any reason why we shouldn't do it?
  • No.
  • Seems like a bug.
  • Let's do it
  • Wait.
  • Ehhh.
  • But.
  • Actually.
  • It's arguable.

Conclusion: No.

(Reason: freshness checking was to catch issues for things like options bags - this sounds more like a potential case for exact types)

@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label Dec 17, 2016
@Igorbek
Copy link
Contributor

Igorbek commented Dec 20, 2016

12/16/2016 ?

@RyanCavanaugh RyanCavanaugh changed the title Design Meeting Notes, 11/16/2016 Design Meeting Notes, 12/16/2016 Dec 20, 2016
@RyanCavanaugh
Copy link
Member

You don't 0-index your months? 😉

@aaronbeall
Copy link

aaronbeall commented Mar 21, 2017

Is there any reason why we shouldn't do it?
No.
Seems like a bug.
Let's do it
Wait.
Ehhh.
But.
Actually.
It's arguable.
Conclusion: No.

(Reason: freshness checking was to catch issues for things like options bags - this sounds more like a potential case for exact types)

Can this reasoning be elaborated on? Can it be re-considered at this point? What does "exact types" mean in this case? How does this answer "is there any reason not to do it"?

Forgive me if I'm being annoying for bringing this issue up in more than one thread, but it's still a big problem for me (and I see even @sandersn thought it looked like an oversight, which I was quite convinced of myself when I ran into it!)

I'll elaborate on my use case, which is straight from the Redux documentation on using object spread.

Say I have an object like this:

interface Todo {
  id: string;
  text: string;
  completed: boolean;
} 

With React/Flux/Redux and other immutable object updating code we want to update this object by replacing it completely, copying over unchanged keys and overwriting specific keys. A very common pattern to do this in JS is to use Object.assign or object spread, as Redux docs show us:

function completeTodo(todo: Todo): Todo {
  return { ...todo, completed: true }
}

I dreamed for a long time of using object spread in TypeScript to do this (as my Babel colleagues have been using this approach for awhile), only to find out its design is not very safe and I probably won't get to use it much. For example, if I use the above code (which does work) and then refactor my interface:

interface Todo {
  id: string;
  text: string;
  isComplete: boolean;
}

I should have errors all over my code (and I will have errors all over my app at runtime!), but I have none, because ...todo will satisfy the existing interface (that's fine) and all my incorrect uses of complete are not raised as errors. Had I used "exact types" of course it would work:

return { id: todo.id, text: todo.text, complete: true }

But this is a much worse pattern and more error prone, I don't think I need to explain why. Besides, it's not idiomatic JS -- using object spread is (or Object.assign).

Even if we take the refactoring story away, simply writing code with the current object spread design is susceptible to typos, causing unexpected runtime behavior without compile warnings:

{...todo, competed: true} // no compile error, but its wrong and is going to lead us down a debugging trail

In general, I just don't see how this can be considered an error:

{ unknown: "foo" }

But this is not:

{ unknown: "foo", ...stuff }

Maybe the explanation is there, but I don't get it.

@DanielRosenwasser
Copy link
Member Author

@aaronbeall exact types refers to the idea of an object type that strictly rejects any type with more properties than have been declared. For example:

class Dog {
  name: string;
  age: number;
}

interface Nameable {
  name: string;
}

declare let d: Dog;

declare function callOver(thingWithName: Nameable): void;

callOver(d);

If we had the concept of exact types in TS, and Nameable was declared as an exact type, then TypeScript would error.

In this design meeting, we were conservative - we decided not to add the excess property checking until we understood what our feelings were around exact types. We can always reconsider this again, but I don't think we're significantly closer on exact types for what it's worth.

@aaronbeall
Copy link

aaronbeall commented Mar 21, 2017

Thanks for the explanation of exact types, @DanielRosenwasser. I guess my confusion comes from the fact that excess property checks seem to already exist for object literals:

const o: { a: string } = { a: "a", b: "b" }; // Object literal may only specify known properties, and 'b' does not exist in type '{ a: string; }'.

But if it contains a spread the same part of the expression that was checked is no longer checked.

const o: { a: string } = { a: "a", b: "b", ...stuff }; // no errors

I really thought this was an oversight, not an intentional design. I take it there is something fundamentally different about an object literal expression that contains a spread somewhere in it, but it doesn't really make sense to me.

@RyanCavanaugh
Copy link
Member

One thing is that because the expression being spreaded might be of a subtype of its declared type, you're already potentially including in "extra" keys. So in that sense, we know we can't know if there are extra properties or not, and you might need to "override" one of those keys to have some different value for whatever reason.

@aaronbeall
Copy link

aaronbeall commented Mar 21, 2017

You mean you wouldn't be able to do this?

const stuff: { b: string; } = { b: "b" };
const o: { a: string; } = { ...stuff, b: "foo" };

My first thought is that's exactly what I'd expect. If you want to throw a bunch of random "stuff" onto a type and override some of those things "for whatever reason", I would think I'm using the wrong type to begin with. In my own use case I can't think of reason I'd do that. But I see how that's a less "conservative" approach. I imagine its overly complex to just include the spread props as known props, but otherwise check for unknown props? I honestly wouldn't care either way, it doesn't matter to my use case. The thing I'm hung up on is explicitly trying to assign an unknown prop (regardless of what else is in the literal).

@JRGranell
Copy link

We're trying to migrate to TS with some of our react / redux projects which makes heavy use of object spread and have come across the issue.

I understand the points of @RyanCavanaugh but as a layman looking at this @aaronbeall has explicitly typed the const 'o' in the example above.

Regardless of the properties / typing of 'stuff' 'o' cannot / should not have the property 'b'? Is there anyway I can benefit from typing in this scenario in order to prevent invalid properties?

Thanks,

@wclr
Copy link

wclr commented Apr 6, 2017

I could not follow all the justifications made in the notes and comments, but what I still see inconsistency.

  let a = { x: 1, y: 2 }
  let b = { t: 1, r: 2 }
  
  type AandB = typeof a & typeof b

  // can add extra props with stpred
  let c: AandB = {
    ...a, ...b,
    extra: 1 // no error
  }

  // can not overide props with sperad
  let cOverrideX: AandB = { //error x incompatilble
    ...a, ...b,
    x: '1'
  }

  // can add extran props and override with Object.assgin/merge  
  let cAssgin: AandB =
    Object.assign(
      a, b,
      { x: 'x', extra: 1 }, // no error, can be extra props, override props types
    )

  // plain explicit version
  let cPlain: AandB = {
    x: a.x,
    y: a.y,
    t: b.t,
    r: b.r,
    extra: 1 // error
  }

So do you propose if one need strict type checking on object structure and props that one should be super explicit and do not use spread or assgin/merge methods but just go verbose and straightforward?

Could some one bring a real case examples that show benifits of current spread/assgin/merge behaviour?

@mikew
Copy link

mikew commented May 9, 2017

Another person trying to convert Redux code to TypeScript, spreads are all over the reducers. Not having type information is definitely unfortunate here, since it's one of the key areas where type hints are beneficial.

@sandersn
Copy link
Member

sandersn commented May 9, 2017

Please upvote #12936, the exact types issue, since that should be the correct fix for this problem. (It only has 6 upvotes right now!)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

8 participants