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

Browser Notification type definition incorrect #14701

Closed
chris-ls opened this issue Mar 17, 2017 · 9 comments · Fixed by microsoft/TypeScript-DOM-lib-generator#438
Closed

Browser Notification type definition incorrect #14701

chris-ls opened this issue Mar 17, 2017 · 9 comments · Fixed by microsoft/TypeScript-DOM-lib-generator#438
Assignees
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this

Comments

@chris-ls
Copy link

TypeScript Version: 2.2.1

Looking at lib.es6.d.ts we have

interface Notification extends EventTarget {
    // ...
    readonly permission: string;
    // ...
}

and

declare var Notification: {
    prototype: Notification;
    new(title: string, options?: NotificationOptions): Notification;
    requestPermission(callback?: NotificationPermissionCallback): Promise<string>;
}

The permission string is a static property on the Notification class.
i.e. We should have:

declare var Notification: {
    prototype: Notification;
    readonly permission: string;
    new(title: string, options?: NotificationOptions): Notification;
    requestPermission(callback?: NotificationPermissionCallback): Promise<string>;
}

and it should be removed from the Notification Interface definition.

@NNemec
Copy link

NNemec commented Mar 22, 2017

Just stumbled over the same issue a today...

NNemec added a commit to NNemec/whiteout-mail that referenced this issue Mar 22, 2017
NNemec added a commit to NNemec/whiteout-mail that referenced this issue Mar 22, 2017
NNemec added a commit to NNemec/whiteout-mail that referenced this issue Mar 22, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Mar 22, 2017

Seems we are also missing maxActions as a static property on Notification

@mhegazy mhegazy added Help Wanted You can do this Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Mar 22, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Mar 22, 2017

PRs welcomed. You can find more information about contributing lib.d.ts fixes at https://github.com/Microsoft/TypeScript/blob/master/CONTRIBUTING.md#contributing-libdts-fixes.

@mhegazy mhegazy added this to the Community milestone Mar 22, 2017
NNemec added a commit to NNemec/whiteout-mail that referenced this issue Mar 22, 2017
@csgulyas
Copy link

I think this would require modifying browser.webidl.xml in the TSJS-lib-generator which the readme says don't, because it comes from Edge

@NNemec
Copy link

NNemec commented Mar 31, 2017

Does that mean that the actual bug is in Edge? Any hope to get it fixed there?
Or is the bug in Typescript using that file in a way that is not intended by Edge?

@csgulyas
Copy link

csgulyas commented Mar 31, 2017

I'm not sure how browser.webidl.xml comes into existence, if it is a result of an extraction process from Edge, or Edge simply interprets it differently, but locally I tried to modify the file and re-generate dom.generated.d.ts.

I removed permission from the properties node and added it as a constant:

<interface name="Notification" extends="EventTarget">
	  <constants>
		<constant name="permission" type="NotificationPermission" />
	  </constants>
...

The output was more along the lines of what OP suggested

interface Notification extends EventTarget {
    ...
    readonly permission: NotificationPermission;
}

declare var Notification: {
    prototype: Notification;
    new(title: string, options?: NotificationOptions): Notification;
    readonly permission: NotificationPermission;
    requestPermission(callback?: NotificationPermissionCallback): Promise<NotificationPermission>;
}

I don't think the build tool supports removing things from the interface while keeping it in the var declaration, but I just started tinkering with it so I might wrong.

rex-ya added a commit to rex-ya/TypeScript-2 that referenced this issue Sep 2, 2017
Issue Fixed.

microsoft#14701 Browser Notification type definition incorrect
@kvetis
Copy link

kvetis commented Jan 10, 2018

@csgulyas Yes, you've identified the issue correctly. The browser.webidl.xml includes incorrect types. The files comes from the Edge team. There is an effort to change this to WHATWG currently underway. If I understand correctly, this would allow the typings to be generated from WHATWG instead of Edge standards.

That, I think, should fix this issue, altough there still is a lot of work ahead. A huge thanks to people working on that.

Also related to #3027

@akoidan
Copy link

akoidan commented Jul 2, 2018

Which version fixes this? "typescript": "^2.9.1",

TS2339: Property 'permission' does not exist on type '{ new (title: string,
options?: NotificationOptions): Notification; prototype: Notification; requ...'.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 2, 2018

Should be in typescript@3.0

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jul 2, 2018
@mhegazy mhegazy added this to the TypeScript 3.0 milestone Jul 2, 2018
@mhegazy mhegazy self-assigned this Jul 2, 2018
@mhegazy mhegazy added the Fixed in TSJS repo Fix merged in https://github.com/Microsoft/TSJS-lib-generator, but not ported yet label Jul 2, 2018
@mhegazy mhegazy reopened this Jul 2, 2018
@mhegazy mhegazy modified the milestones: TypeScript 3.0, TypeScript 3.1 Jul 3, 2018
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Fixed A PR has been merged for this issue labels Jul 25, 2018
@mhegazy mhegazy removed the Fixed in TSJS repo Fix merged in https://github.com/Microsoft/TSJS-lib-generator, but not ported yet label Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants