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

Allow accessors in ambient class declarations #32787

Merged
merged 2 commits into from
Aug 9, 2019

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Aug 9, 2019

As part of the effort to support ECMAScript class fields, we will need to roll out features to the TypeScript compiler in phases. One of the key areas we need to address is the ability to properly distinguish between a "field" (a.k.a. a "Property Declaration" in TypeScript parlance), and an "accessor" (a getter or setter declaration). The reason we need to have these definitions cleanly separated is so that we can eventually issue error messages when you define a field on a superclass and an accessor on a subclass with the same name (or vice versa).

However, this is problematic today for several reasons:

  • Subclasses currently can override a property declaration from a superclass with an accessor on a subclass:
    var Map: MapConstructor;
    interface Map<K, V> {
       readonly size: number;
       ...
    }
    
    class MyMap extends Map {
      get size() {
        return super.size + 1;
      }
      
    }
  • Declaration emit for class declarations converts get/set accessor pairs into property declarations and get-only accessors into readonly property declarations:
    // input: main.ts
    class MyClass {
      get x(): number { return 1; }
      set x(value: number) {  }
    }
    
    // output: main.d.ts
    declare class MyClass {
      x: number;
    }
  • Interfaces can specify property declarations, but can't specify accessors.

This PR seeks to address some of these issues:

  • Ambient Class declarations may now specify getters/setters.
  • Code fixes for implementing a Class as if it were an interface, or extending an abstract class will now add implementations for getters/setters instead of fields when the implemented member is a getter or setter.
  • Getters and Setters defined on an ambient class are preserved when emitting an output declaration file.

This PR does not change the following behavior:

  • Getters and Setters are still not permitted on interfaces.
  • Getters and Setters on a non-ambient class declaration will continue to be emitted as a property declaration.
  • It is still not an error to have a subclass and superclass have a mismatch in a definition of a property that is either an accessor or a property declaration.

We are not changing the declaration emit for getters/setters immediately so that declaration files built against existing TypeScript code prior to this feature can still be used with earlier versions of the TypeScript compiler. We do plan to change the declaration emit in a future release, which will give users a transitional period where they can become accustomed to the new syntax.

Examples:

declare class C {
  get x(): number;
}

Supersedes #32772
Related #27644
Fixes #19703

@rbuckton
Copy link
Member Author

rbuckton commented Aug 9, 2019

@weswigham, @RyanCavanaugh, @sandersn, @ahejlsberg, @DanielRosenwasser for review since GitHub's suggested reviewers still don't seem to be working.

@rbuckton rbuckton force-pushed the allowAccessorsInAmbientClassesOnly branch from 7b17de1 to 3d7d144 Compare August 9, 2019 21:01
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see this expanded to ObjectLiteral and Interface types in the near future, but I guess it's fine to start here until we determine how implementing those is going to work. I can't really come up with a reason reason for not allowing it in classes beyond "we liked the idea of deleting more information in the declaration file".

@rbuckton
Copy link
Member Author

rbuckton commented Aug 9, 2019

I'd like to see this expanded to ObjectLiteral and Interface types in the near future

That's what #32772 does, but after discussing with @RyanCavanaugh we decided to postpone that until at least 3.7.

Co-Authored-By: Wesley Wigham <wewigham@microsoft.com>
@qwertie
Copy link

qwertie commented Feb 24, 2020

As one of the people getting this error in a d.ts file on npm...

get prop(): T; // inside a "export declare class" declaration
    ^^^^
    error TS1086: An accessor cannot be declared in an ambient context.

...I'm wondering why it is necessary to "issue error messages when you define a field on a superclass and an accessor on a subclass with the same name (or vice versa)" and why this breaking change was done during a point-release change (3.7) instead of a major version change (4.0).

(Plus, what's an "ambient context"?)

@weswigham
Copy link
Member

this breaking change was done during a point-release change (3.7) instead of a major version change (4.0).

Because TS isn't semver and every minor version of TS contains breaks to some degree, just not big ones.

Plus, what's an "ambient context"

Declaration file or "declare"'d statement.

I'm wondering why it is necessary to "issue error messages when you define a field on a superclass and an accessor on a subclass with the same name (or vice versa)

Accessors are on the prototype, TS old fields were on the actual instance; this mismatch could lead to occasionally surprising behavior (esp with useDefineForClassFields); so those cases were flagged as errors.

@ExE-Boss
Copy link
Contributor

This also fixed #9907.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Implement interface' quick fix generates bodies in ambient contexts
4 participants