Add firefox-webext-browser types for FireFox WebExtension Development by jsmnbom · Pull Request #21749 · DefinitelyTyped/DefinitelyTyped

jsmnbom · 2017-11-25T23:04:07Z

template

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Not tested using typescript code, but the types have been tested using WebStorm's type checking engine on various javascript snippets
Follow the advice from the readme.
Think i did everything properly
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).
Lint passes, note the rules in tslint.json though

If adding a new definition:

The package does not provide its own types, and you can not add them.
If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
Create it with dts-gen --dt, not by basing it on an existing project.
tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

questions

These definitions have been autogenerated from (originally chrome) schema files. Should I commit the generator script? And if so what should it be called?
I had huge issues with line 1992 and 1994 in index.d.ts since it describes a function with a reserved name. Is there any way to actually do this properly, instead of this (very bad) workaround?

dt-bot · 2017-11-25T23:04:09Z

types/firefox-webext-browser/index.d.ts

Checklist

is correct naming convention?
has a test file? (firefox-webext-browser-tests.ts or firefox-webext-browser-tests.tsx)

RyanCavanaugh · 2017-11-29T19:10:36Z

Generally the best is to have a separate repo (yours) for the generator script and link that repo in the comments of the file
Sometimes it's possible to completely rewrite the way the declaration is written to accomplish this. For example

namespace foo.bar {
  export function default(): void;
}

is illegal, but is equivalent to the legal declaration

namespace foo {
  export const bar: {
    default(): void;
  }
}

There are certain combinations that can't be expressed that way; see microsoft/TypeScript#9846

mhegazy · 2017-11-29T21:45:10Z

this is legal:

declare namespace foo.bar {
    function _default(): void;
    export { _default as default };
}

- Rename generated enums not present in schemas so that they have an underscore first in hopes of it being seen as private (and therefore not available to code) - Fix browser.bookmarks.import/export as per PR comments

jsmnbom · 2017-11-30T10:26:12Z

@RyanCavanaugh Thanks for those answers, I've gone ahead and created a repository for the generator and put it in a comment.
@mhegazy Thanks that seems to work (at least it passes lint, webstorm can't seen to understand the export {y as x} properly)

Should be good for review now.

rpl

@bomjacob hey! this is amazing, thank you a lot for getting this started \o/

I had no previous experience with typescript until now, and so it has been also a good opportunity to look into it for my first time ;-)

I have not been able to look too deep into all the API namespaces yet, but I've added an initial round of comments that could be already useful.

rpl · 2017-12-02T18:38:41Z

+        stdio = "stdio"
+    }
+
+    enum _NativeManifestType {


This _NativeManifestType has been declared two times (probably because of how the generator script from https://github.com/bomjacob/definitelytyped-firefox-webext-browser is handling this schema file https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/toolkit/components/extensions/schemas/native_manifest.json#25,55).

But this enum is only used in the NativeManifest type which is not exposed to any API method (iirc we just use it internally to validate this kind of manifest JSON files, but they are not used in any of the APIs exposed to the extension code), and so I think that the generator script could just ignore them when parsing the schema files and/or omit their types from the generated typescript type definitions.

I've chosen just to omit the NativeManifest type for now

rpl · 2017-12-02T18:43:09Z

+// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
+// TypeScript Version: 2.4
+// Generated using script at github.com/bomjacob/definitelytyped-firefox-webext-browser
+


it could be probably reasonable to also define browser as a property of the Window object, like the existent chrome types from this repo are doing:

DefinitelyTyped/types/chrome/index.d.ts

Lines 9 to 14 in 0128360

////////////////////

// Global object

////////////////////

interface Window {

chrome: typeof chrome;

}

I'm not 100% sure if it is needed to be able to define the browser property inside the Window interface, but it is worth to mention that chrome tsconfig.json also include "dom" in the libs property:

DefinitelyTyped/types/chrome/tsconfig.json

Lines 4 to 7 in 0128360

"lib": [

"es6",

"dom"

],

rpl · 2017-12-02T19:09:52Z

+    const onAlarm: EventListener<(name: Alarm) => void>;
+}
+
+declare namespace browser.manifest {


There is no browser.namespace API namespace exposed to the extensions code, but some of this types are used in other APIs, it could be nice if it is possible to include the needed types definitions without an explicit browser.manifest namespace, which I guess would be suggested in auto-completion because it exists in these types definitions, am I right? or, if this namespace can't be easily removed, another option could be to rename it to browser._manifest)

I assume you mean that there is no browser.manifest namespace exposed?

I've renamed it to _manifest for now...
I've tried to see if there is any way to make some functions or types "private" so that they wouldn't show up in autocomplete, but I've found nothing so far.

rpl · 2017-12-02T19:12:29Z

+// TypeScript Version: 2.4
+// Generated using script at github.com/bomjacob/definitelytyped-firefox-webext-browser
+
+interface EventListener<T extends (...args: any[]) => any> {


Ah, this is very nice!

I think that it would be better to rename it, e.g. WebExtEventListener, so that its name doesn't clash with the EventListener type defined as part of "dom" (and this is also shown when running the linting once you include "dom" in the tsconfig.json libs)

rpl · 2017-12-02T20:11:57Z

+        };
+    }
+
+    type KeyName = string | string | string;


I guess that the generator scripts generates this type from https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/browser/components/extensions/schemas/commands.json#9-25
which contains 3 possible patterns that a command included in the manifest.json should match (it should match 1 of them).

An option to fix it in the generator script could be to clean up the results that we get from processing the "choices" in the schemas, by removing any type that has been included more than ones in the result.

rpl · 2017-12-02T22:32:44Z

+        type?: ViewType;
+        windowId?: number;
+        tabId?: number;
+    }): object/*Window*/[];


object could be probably be Window here if "dom" is included in tsconfig.json.

rpl · 2017-12-02T22:55:45Z

+
+    function getPlatformInfo(): Promise<PlatformInfo>;
+
+    function getPackageDirectoryEntry(): Promise<object/*DirectoryEntry*/>;


getPackageDirectoryEntry is not supported on Firefox:

https://searchfox.org/mozilla-central/rev/1b7f664804adf9b0393df94e9ec92043ceb707d3/toolkit/components/extensions/schemas/runtime.json#435-436

In general every API method, type and property which is marked as "unsupported": true in the schema, it is something that is available in the original Chrome schema file but it is not in the Firefox implementation.

it could be reasonable to keep this "unsupported" property into account in the generator script, one option can be to omit what is unsupported from the final generated types.

Another option could be to make them optional properties in the generated typescript type definitions, so that the "typescript type checking" would raise an error if the "unsupported" method (or property) is used without doing some kind of "feature detection" (e.g. this is also true for the Firefox for Android schema, which is some cases doesn't support yet all the features supported in Firefox for Desktop, and so in the Firefox for Android schema some property may be marked as unsupported, https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/schemas).

Okay, I live the 2nd approch the most, so I've made the generator script treat unsupported as optional.
It's working for properties, functions and events, I am unsure if we can make class, enum, type and interface optional though.

rpl · 2017-12-02T23:01:04Z

+        typed = "typed",
+        auto_bookmark = "auto_bookmark",
+        auto_subframe = "auto_subframe",
+        manual_subframe = "manual_subfram


browser.contextMenus should also contain the API methods provided by browser.menus (the related API schema file is probably the only one that uses an "$import" property, which imports the content of the "menus" namespace into the "contextMenus" namespace)

rpl · 2017-12-02T23:08:04Z

+        typed = "typed",
+        auto_bookmark = "auto_bookmark",
+        auto_subframe = "auto_subframe",
+        manual_subframe = "manual_subfram


browser.menusInternal is not not exposed to the extension code as a valid API namespace, but the OnClickData type defined here seems to be needed because it is used in the browser.menu.update API method

(to be fair, the two menu.OnClickData and menusInternal.OnClickData types seems to be similar but not in sync in the Firefox API schema, I'm going to take a deeper look to how we are using them internally).

So would an okay solution for now be to maybe rename menusInternal to _menusInternal?

rpl · 2017-12-02T23:17:54Z

@@ -0,0 +1,2 @@
+// No tests yet
+true;


While I was experimenting a bit with this (I had not previous experience with typescript or DefinitelyTyped until now), I started to put something in this file, it looks a pretty useful way to test the generated types (especially if you want to be sure that the types are still generated correctly while tweaking the generator script), here is some example tests that I added into it:

browser.nonexistentNS.unknownMethod(); // $ExpectError // Various API types tests browser.runtime.getManifest(); // $ExpectType ManifestType

See commit history at jsmnbom/definitelytyped-firefox-webext-browser@71548e2...b7e3c09 for full list of changes.

jsmnbom · 2017-12-07T07:10:03Z

@bowdenk7 I'm not sure this PR was quite ready for merging yet... For one, it wasn't been thoroughly reviewed by anyone with deep knowledge of the API.
@rpl even said he would follow up with some more comments when he had time.

FranklinYu · 2017-12-11T15:55:03Z

@bomjacob The DefinitelyTyped team prefers fast iteration. If any modification is required according to later review, it is desired to create a new pull request. AFAIK.

Add firefox-webext-browser types for FireFox WebExtension Development

1dc3986

typescript-bot added the New Definition This PR creates a new definition package. label Nov 26, 2017

RyanCavanaugh mentioned this pull request Nov 29, 2017

Allow quoted names in ambient contexts microsoft/TypeScript#9846

Closed

Fix some smaller things

d7ca842

- Rename generated enums not present in schemas so that they have an underscore first in hopes of it being seen as private (and therefore not available to code) - Fix browser.bookmarks.import/export as per PR comments

rpl reviewed Dec 2, 2017

View reviewed changes

Changes per @rpl review

27a03d6

See commit history at jsmnbom/definitelytyped-firefox-webext-browser@71548e2...b7e3c09 for full list of changes.

bowdenk7 merged commit d456040 into DefinitelyTyped:master Dec 6, 2017

ghost mentioned this pull request Dec 8, 2017

Typescript typings mozilla/webextension-polyfill#78

Closed

jsmnbom mentioned this pull request Dec 28, 2017

[firefox-webext-browser] General fixes and updates #22542

Merged

7 of 8 tasks

JReinhold mentioned this pull request Feb 11, 2018

Make it work with other browsers JReinhold/google-inbox-to-trello#10

Open

1 of 5 tasks

	////////////////////
	// Global object
	////////////////////
	interface Window {
	chrome: typeof chrome;
	}


		function getPlatformInfo(): Promise<PlatformInfo>;

		function getPackageDirectoryEntry(): Promise<object/DirectoryEntry/>;

Conversation

jsmnbom commented Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

template

questions

Uh oh!

dt-bot commented Nov 25, 2017

Uh oh!

RyanCavanaugh commented Nov 29, 2017

Uh oh!

mhegazy commented Nov 29, 2017

Uh oh!

jsmnbom commented Nov 30, 2017

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmnbom commented Dec 7, 2017

Uh oh!

FranklinYu commented Dec 11, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jsmnbom commented Nov 25, 2017 •

edited

Loading