Skip to content

Require either async override or override async (don't allow both) #43606

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

Closed
Skalman opened this issue Apr 9, 2021 · 4 comments Β· Fixed by #43660
Closed

Require either async override or override async (don't allow both) #43606

Skalman opened this issue Apr 9, 2021 · 4 comments Β· Fixed by #43660
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@Skalman
Copy link

Skalman commented Apr 9, 2021

Bug Report

πŸ”Ž Search Terms

class async override

πŸ•— Version & Regression Information

  • This changed between versions 4.2.0 and nightly

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

class Base {
    method1() {}
    method2() {}
}

class Test extends Base {
    public override async method1() {
        return Promise.resolve(1);
    }

    public async override method2() {
        return Promise.resolve(1);
    }
}

πŸ™ Actual behavior

No errors

πŸ™‚ Expected behavior

Possibly error on either override async or async override.

My understanding (from #43533) is that you prefer requiring a specific modifier order. If that's the case, I would have expected an error here.

@dsherret
Copy link
Contributor

dsherret commented Apr 9, 2021

For some more information on which one should be chosen if one should be chosen, C# supports both forms, but override async is used about 12x more than async override in open source projects. Also, doing override async would keep in line with having the readonly keyword appear after override as mentioned in that linked issue, Additionally, override async is winning big time in this poll I did.

Right now, it seems like the declare keyword is the only modifier whose order isn't enforced.

@fbartho
Copy link

fbartho commented Apr 9, 2021

just like public/private, override is something of an internal annotation (for implementors), while async is a more external annotation (for consumers).

Therefore, it makes sense to me to spell it as public override readonly async someMethod() which follows a consistent direction of moving along a spectrum from implementor-relevant towards consumer-focused (left-to-right).

@fbartho
Copy link

fbartho commented Apr 9, 2021

To put it another way:

  • async applies to the right to someMethod()
  • readonly applies to the instance (what you can do to an instance)
  • override and public both apply outwards to the class or meta level

So working from the outside in meta -> instance -> method becomes public override readonly async someMethod()

(Not sure if this phrasing makes more sense to people)

@andrewbranch andrewbranch added the Bug A bug in TypeScript label Apr 9, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.3.1 milestone Apr 9, 2021
@andrewbranch andrewbranch self-assigned this Apr 9, 2021
@andrewbranch
Copy link
Member

The order of override and static is also not currently enforced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants