Convert generation script to TS, and update the source webidl #383

mhegazy · 2018-03-01T18:18:58Z

This change includes two parts:

A complete rewrite of the generator script:

Moving from F# to TS to simplify contribution and code review
Using JSON files for all inputs instead of xml

An update to the Edge webidl file

The new file is more inline with the standard webidl format, which will allow us to reuse the script to generate directly from webidl
The new file comes in JSON format with no need for preprocessing
The new file has multiple changes that bring us closer to the spec, with removals of MS-specific types and adding new definitions

A note about methodology:
I have converted the original F# script to TS first, maintain the input file source, and testing as I went to ensure the output was the same. when that was complete, i then plugged in the new file definition file and continued from there. Unfortunately the new input file is different enough from the new one that it made splitting this change into two more expensive.

A note for reviewers:
There are two things to look out for, first and foremost the changes in the generated files, and second the generation script.

Testing:

I have run the local TS tests, as well as the RWC tests. and addressed the issues that came up,
I still need to run the user tests and DefinitelyTyped on the new generated files.

ghost · 2018-03-02T22:19:51Z

package.json

+  "main": "index.js",
+  "scripts": {
+    "build": "tsc --p ./tsconfig.json && node ./lib/index.js",
+    "baseline-accept": "copy /Y generated\\* baselines\\",


copy won't work on linux, could you use a node script for this instead?

ghost · 2018-03-02T22:24:15Z

src/helpers.ts

+export function filter(obj: any, fn: (o: any, n: string | undefined) => boolean) {
+    if (typeof obj === "object") {
+        if (Array.isArray(obj)) {
+            const newArray: any[] = [];


return mapDefined(obj, e => fn(e, undefined) ? filter(e, fn) : undefined);

ghost · 2018-03-02T22:26:27Z

src/helpers.ts

+    return src;
+}
+
+function mergeNamedArrays<T extends { name: string }>(srcProp: T[], targetProp: T[]) {


This is only ever called on an any[], so might as well just make the args { name: string }[]...

i do not think there is harm from the extra polymorphism

ghost · 2018-03-02T22:29:25Z

src/index.ts

+import * as path from "path";
+import { filter, merge, filterProperties, mapToArray, distinct, map, toNameMap } from "./helpers";
+
+enum Flavor {


Nit: const enum

ghost · 2018-03-02T22:30:57Z

src/index.ts

+        function getEventHandler(i: Browser.Interface) {
+            const ownEventHandler =
+                i.properties
+                    ? mapToArray(i.properties.property).filter(p => p["event-handler"]).map(p => ({


Nit: map<EventHandler> or there is no contextual type.

This could be:

return !i.properties ? [] : mapDefined<Browser.Property, EventHandler>(mapToArray(i.properties.property), p => { const eventName = p["event-handler"]!; if (eventName === undefined) return undefined; const eType = eNameToEType[eventName] || defaultEventType; const eventType = eType === "Event" || dependsOn(eType, "Event") ? eType : defaultEventType; return { name: p.name, eventName, eventType }; });

ghost · 2018-03-02T23:14:24Z

src/index.ts

+    // Some params have the type of "(DOMString or DOMString [] or Number)"
+    // we need to transform it into [“DOMString", "DOMString []", "Number"]
+    function decomposeTypes(t: string) {
+        return t.replace(/[\(\)]/g, "").split(" or ");


Maybe to be a bit safer:

if (t[0] !== "(") return [t]; assert(t[t.length - 1] === ")"); return t.slice(0, t.length - 1).split(" or ");

We should remove this code path altogether anyways, just leaving it for now until we consume other webidl files.

ghost · 2018-03-02T23:16:43Z

src/index.ts

+        return t.replace(/[\(\)]/g, "").split(" or ");
+    }
+
+    function createTextWriter(newLine: string) {


Might be better to print to nodes eventually?

do not think so. so far our transformations are not that complicated..

ghost · 2018-03-02T23:19:51Z

src/index.ts

+    }
+
+    function makeArrayType(elementType: string): string {
+        return elementType.indexOf("|") > -1 ? `(${elementType})[]` : `${elementType}[]`;


.includes("|")

ghost · 2018-03-02T23:22:07Z

src/index.ts

+    }
+
+    function convertDomTypeToTsTypeWorker(obj: Browser.Typed): { name: string; nullable: boolean } {
+        let type;


Looks like obj["type-original"] is never used?

This function seems to be equivalent to:

if (typeof obj.type === "string") { const name = covertDomTypeToTsTypeSimple(obj.type); const typeArg = obj.subtype && convertDomTypeToTsType(obj.subtype); return { name: typeArg ? name === "Array" ? makeArrayType(typeArg) : `${name}<${typeArg}>` : name, nullable: !!obj.nullable, }; } else { assert(!obj.subtype); const types = obj.type.map(convertDomTypeToTsTypeWorker); return { name: types.map(t => t.name).join(" | "), nullable: types.some(t => t.nullable), }; }

yes. i wanted to leave it for now to see what other idl files have

Could still apply the above refactoring though?

ghost · 2018-03-02T23:22:57Z

src/index.ts

+    }
+
+    function covertDomTypeToTsTypeSimple(objDomType: string): string {
+        if (typeof objDomType !== "string") {


Was this debugging code?

…g type names on DT

mhegazy · 2018-03-05T22:18:16Z

@weswigham and @andy-ms any more comments?

kenchris · 2018-03-06T09:39:54Z

Btw, just wanted to point out (not sure if relevant) that WebIDL is seeing many changes lately after being abandoned for quite some years. This happens on the editors draft though and it not in stable yet:

https://heycam.github.io/webidl/

* Fixes in preparation of lib changes See microsoft/TypeScript-DOM-lib-generator#383 * Fix breakes * revert changes to knuddels-userapps-api * Fix compressedTexSubImage2D overload * Fix event emitter methods * Remove duplicate declaration * Fix ScreenOrientation.type

ghost · 2018-03-06T21:35:46Z

src/index.ts

+import { Flavor, emitWebIDl } from "./emitter";
+
+function emitDomWorker(webidl: Browser.WebIdl, knownWorkerTypes: Set<string>, tsWorkerOutput: string) {
+    const worker: Browser.WebIdl = {


This is an odd name, would call it converted. Also, this initializer is identical to const result: Browser.WebIdl later, so could factor those out to a getEmptyWebIdl() function.

it is the filtered webidl file to represent the webworker APIs, simiar to the other one, browser.

ghost · 2018-03-06T21:36:12Z

src/index.ts

+
+    const result = emitWebIDl(worker, Flavor.Worker);
+    fs.writeFileSync(tsWorkerOutput, result);
+    return;


unnecessary return

it is intentinal to get an error if i have any code mixed with the helpers.

ghost · 2018-03-06T21:36:37Z

src/index.ts

+
+    const result = emitWebIDl(browser, Flavor.Web);
+    fs.writeFileSync(tsWebOutput, result);
+    return;


unnecessary return

same as above

ghost · 2018-03-06T21:37:25Z

src/index.ts

+
+        if (obj["callback-functions"]) result["callback-functions"]!["callback-function"] = filterProperties(obj["callback-functions"]!["callback-function"], (cb) => !(template["callback-functions"] && template["callback-functions"]!["callback-function"][cb.name]));
+        if (obj["callback-interfaces"]) result["callback-interfaces"]!.interface = filterInterface(obj["callback-interfaces"]!.interface, template["callback-interfaces"] && template["callback-interfaces"]!.interface);
+        if (obj.dictionaries) result.dictionaries!.dictionary = filterDictinary(obj.dictionaries.dictionary, template.dictionaries && template.dictionaries.dictionary);


typo: dictionary (in filterDictinary)
Also, this could be if (obj.dictionaries && template.dictionaries) instead of having the null check at the start of filterDictinary

I need to assign it something, so having them in the functions seems easier to read.

ghost · 2018-03-06T21:40:24Z

src/emitter.ts

+    "AudioContext": { extendType: ["OfflineContext"], memberNames: new Set(["suspend"]) },
+    "HTMLCollection": { extendType: ["HTMLFormControlsCollection"], memberNames: new Set(["namedItem"]) },
+};
+const evetTypeMap: Record<string, string> = {


Typo: event

ghost · 2018-03-06T22:05:59Z

src/emitter.ts

+    function emitProperties(prefix: string, emitScope: EmitScope, i: Browser.Interface, conflictedMembers: Set<string>) {
+        // Note: the schema file shows the property doesn't have "static" attribute,
+        // therefore all properties are emited for the instance type.
+        if (emitScope !== EmitScope.StaticOnly) {


Nit: double if could be replaced with &&

ghost · 2018-03-06T22:07:05Z

src/emitter.ts

+    function emitMethods(prefix: string, emitScope: EmitScope, i: Browser.Interface, conflictedMembers: Set<string>) {
+        // If prefix is not empty, then this is the global declare function addEventListener, we want to override this
+        // Otherwise, this is EventTarget.addEventListener, we want to keep that.
+        function mFilter(m: Browser.Method) {


Would inline this fn

ghost · 2018-03-06T22:08:31Z

src/emitter.ts

+    }
+
+    /// Emit the properties and methods of a given interface
+    function emitMembers(prefix: string, emitScope: EmitScope, i: Browser.Interface) {


Would move prefix to the end and make it optional.
Also, the only prefix value currently (besides "") is "declare var", so maybe the parameter should be just inNamespace = false.

yeah. i need to do this for all prefixes.. but will do this separately. no need to pass a string in.

ghost · 2018-03-06T22:20:40Z

src/emitter.ts

+        }
+    }
+
+    function emitEventHandlers(prefix: string, i: Browser.Interface) {


not sure how i feel about functions declared in a for loop. will refactor though.

ghost · 2018-03-06T22:22:03Z

src/emitter.ts

+    }
+
+    function emitConstructorSignature(i: Browser.Interface) {
+        const constructor = typeof i.constructor === "object" ? i.constructor : undefined;


Was there some bad input this is avoiding?

no this is cause all objects have a constructor property on them :D

Oh yeah 🤦‍♂️

timocov · 2018-03-26T11:22:18Z

@mhegazy seems that this PR change the type of touch* events (touchstart for example) from TouchEvent to just Event.

See microsoft/TypeScript#22565

mhegazy · 2018-03-26T15:49:34Z

PRs welcomed.

mhegazy added 30 commits February 20, 2018 10:45

Convert input files to json

63022de

Add schema for input

317c27a

Add config files

e80f384

Convert script to TS

b0bdc00

Normalize input files

5ffa5fe

Fix some conversion mistakes

751571f

Make some arrays maps

e7aba4c

Add support for loading comments

2f0d977

Add some types to merge

0426bda

Move source files to a diffrent directory

63e0a7e

revert changes to comments

a82b8cb

normalize addedTypes.json and support overrides

6ddee3e

Add support for removign items

0c0e158

Add support of overriding definitons

c3f0346

Add .js to gitignore

8125a3e

Clean up some remaining diffs

0131cf2

Rremove unused declarations

294a92d

Cleanup

0b9c8a3

Use new input file

bdb44dd

Update types

6e865bc

Refactor

2e4133b

Add support for overrides

80018bb

Add list of known types for workers

bf4d9da

Remove unused function

a2a308b

Better handeling for unknown types

4dca898

Remove unused comments

0480b7c

make event handelers nullable

5f10d60

Add deprecated comments

a255454

Refactor

1106259

Remove index signature on Window

e47cdf6

This was referenced Mar 2, 2018

Fixes in preparation of lib changes DefinitelyTyped/DefinitelyTyped#24019

Merged

lib.dom.d.ts v2.7.2 contains two reportValidity method definitions microsoft/TypeScript#22301

Closed

ghost reviewed Mar 2, 2018

View reviewed changes

mhegazy added 5 commits March 5, 2018 12:35

Remove Window.browser property

e897757

Rename ClientType to ClientTypes to avoid conflicts with exisitin…

3c9471d

…g type names on DT

Use cpx to copy

3e5afdb

Remove iife's

64d4cca

Code review comments

5d59e97

weswigham approved these changes Mar 5, 2018

View reviewed changes

ghost referenced this pull request in DefinitelyTyped/DefinitelyTyped Mar 6, 2018

web-animations-js: Make compatible with TypeScript@2.7 (#23793)

bf37dfd

mhegazy added 4 commits March 6, 2018 13:07

Refator emit function to a diffrent file

bb7a313

Extract helpers outside the main function

967d7e4

cleanup gitignore

0920382

Set private: true

99305e7

ghost reviewed Mar 6, 2018

View reviewed changes

more code review comments

6d46a8b

ghost approved these changes Mar 7, 2018

View reviewed changes

mhegazy mentioned this pull request Mar 7, 2018

Port generated lib files microsoft/TypeScript#22381

Merged

mhegazy merged commit 5484c6e into master Mar 7, 2018

mhegazy deleted the convertToTS branch March 7, 2018 18:08

This was referenced Mar 7, 2018

fix: add escape to CSS #371

Closed

Add WebIDL typedefs by json #338

Closed

Add Encoding Stardard by adding browser.webidl.json #300

Closed

jakearchibald mentioned this pull request Mar 26, 2018

Adding touch event typings #394

Merged

krzysztof-grzybek mentioned this pull request Jun 27, 2018

Remove unused file #519

Closed

Convert generation script to TS, and update the source webidl #383

Convert generation script to TS, and update the source webidl #383

Conversation

mhegazy commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Mar 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Mar 5, 2018

kenchris commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Mar 6, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timocov commented Mar 26, 2018

mhegazy commented Mar 26, 2018

ghost Mar 2, 2018 •

edited by ghost

Loading

ghost Mar 6, 2018 •

edited by ghost

Loading