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, 11/17/2017 #20117

Closed
mhegazy opened this issue Nov 17, 2017 · 7 comments
Closed

Design Meeting Notes, 11/17/2017 #20117

mhegazy opened this issue Nov 17, 2017 · 7 comments
Labels
Design Notes Notes from our design meetings

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Nov 17, 2017

Strict property initialization checks in classes

  • abstract properties should be excluded
  • T extends number | underfed is an error, since we do not know if it is one or the other at instantiation time
  • Enabled under new strict mode flag --strictPropertyInitialization
  • Issues:
  1. names in quotes are not checked:
    class B {
       a: number;  // Error
       "b" : number; // No error
    }
  2. Do we need a new explicit modifier?
    • How do you opt-out for a specific class
      • // @ts-ignore
        • seems dangerous to rely on this, you change the type, and now no errors
      • or put it in "b"
        • too cute..
        • this looks like a bug that we should fix in the future
      • new modifier:
        • kotlin uses lateinit, swift has lazy?
        • bang operator: a!: number
        • extend it to locals as well
          • not allowed on const
          • should be an error to have it on something with an initializer
          • readonly is ok to have ! to allow for external
          • does not amke sense for statics
          • @bterlson is not happy about !
            • but we have it in expression positions already
  3. method calls in the constructor
    • this guards against observing un-initialized properties outside the class, but not inside the class
    • this allows for for init patterns
    • it is also easy to get around this using aliases, or even through super access
  • @DanielRosenwasser, all angular projects, mobx, etc.. that use decorators to initialize properties will have to opt-out of these features
  • you can bang these properties
  • it is possible that in the future to allow decorators to change the type of the declaration, and in that world we would not need to bang
  • this is opt-in at the end of the day, and you can keep the world you are used to
@mhegazy mhegazy closed this as completed Nov 17, 2017
@mhegazy mhegazy added the Design Notes Notes from our design meetings label Nov 17, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 17, 2017

My notes:


Strict Property Initialization Checks in Classes

  • Does adding | undefined to a declaration, do we still do initialization checking within the constructor?
    • No, it's in the domain, allowed not to initialize it.
    • What about with generics that have undefined as a constraint?
      • No, only signals it could be instantiated with undefined
  • What about @decorated property declarations?
    • People can always opt out of this.
    • Code in the RWC doesn't break because they don't opt into strict.
  • Does it make sense to have a modifier for this?
    • ! after the property name.
    • lateinit? Some sort of modifier to assure the compiler it doesn't need to write any checks.
    • Should we error on initialized declarations like a!: number = 10?
      • Probably should let linters take over this.
    • Make it !, it reflects expression level non-null assertion logic.
      • Allow it on locals.
      • But don't allow it on const.
      • Also, probably error when we give an initializer (nix that last point about linters taking this on)
      • But allow it on readonly.
  • What about local methods that can be called from the constructor that might observe uninitialized values.
    • Only so far we can go.

@tinganho
Copy link
Contributor

you can bang these properties

This sounds funny.

Nearly all of our classes are handled by some sort of IoC container.

Sometimes(99% in my case) classes, are just definitions and construction is "inversioned". The IoC container takes care of construction of the class. I don't want the compiler to complain in these type of classes. And providing a flag to opt-out or banging all the properties doesn't sound optimal either.

One suggestion, I can think of, at least for the decorator use case, is to have a modifier on the "definition" class decorator.

In the below example you can see a constructtarget modifier:

export function Entity(definitions?: EntityDefinitions) {
    return constructtarget function(target: any) {
        //...
    };
}

And applying it on entity definitions will not cause any errors:

@Entity()
class User {
    @Column()
    name: string; // no error
    @Column()
    age: number; // no error
}

Example from TypeOrm https://github.com/typeorm/typeorm.

@DanielRosenwasser
Copy link
Member

We discussed decorators that can declare how they modify the target, so it's an option, but others in the meeting felt like doing so shouldn't be a blocker for the flag as it exists today.

@mihailik
Copy link
Contributor

Should this flag hold off from graduating to --strict set?

If it needs to be disabled for a all/large subset of angular code out there, perception of TS as a stable platform might be affected.

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 22, 2017

Should this flag hold off from graduating to --strict set?

we have the infrastructure in place to opt-out of specific --strict flags as needed. --strict --strictPropertyInitialization false. --strict by definition means you are accepting breaking changes between releases. if this is not your intention, then do not use --strict and instead spell out the flags you are interested in, e.g. --noImplictAny --strictNullchecks

@mihailik
Copy link
Contributor

Hope you're right — regardless of technical reasoning you still want gain > pain to proceed

@tinganho
Copy link
Contributor

I think many people would rather bang all properties than setting the flag, to just keep the default "strict" mode. But I hope the TS team can hold this off a bit, certainly a contentious topic. As of right now, it feels like a choice between pest and colera.

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

4 participants