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

Add Web IDL base types + Encoding Standard from their specs #405

Merged
merged 15 commits into from
Apr 5, 2018

Conversation

saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Apr 4, 2018

The new json type file tends to flatten typedefs, is it a requirement?

@@ -0,0 +1,23 @@
dictionary TextDecoderOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the idl file comming from?

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind. found the link :)

src/fetcher.ts Outdated
}

async function fetchIDLs() {
const file = fs.readFileSync(`${__dirname}/../inputfiles/idlSources.json`, { encoding: "utf-8" });
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i would use path.combine here.

Copy link
Contributor

Choose a reason for hiding this comment

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

or consider using require to get it and avoid JSON.parse

async function fetchIDL(source: IDLSource) {
const response = await fetch(source.url);
const dom = new JSDOM(await response.text());
const elements = Array.from(dom.window.document.querySelectorAll("pre.idl:not(.extract),code.idl-code"));
Copy link
Contributor

Choose a reason for hiding this comment

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

totally random, and not related to this change; it would be super cool if in the future we can extract the description from the widl file and put it as a comment on the declaration..

throw new Error("Unsupported IDL type structure");
}

function createEmptyBrowserWebidl(): Browser.WebIdl {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have this function already in index.ts, i would move it helpers and reuse it.

export function convert(text: string) {
const rootTypes = webidl2.parse(text);
const partialInterfaces: Browser.Interface[] = [];
const browser = createEmptyBrowserWebidl();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, browser might not be the best name, consider changing it to webidl

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

A few comments, and the build seems to be failing on a few errors as well.

return {
name: operation.name!,
signature: [{
...convertIdlType(operation.idlType!),
Copy link
Contributor

Choose a reason for hiding this comment

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

should we throe if idlType is not defined?

@saschanaz saschanaz force-pushed the widl branch 2 times, most recently from 0cb7327 to afcc4a1 Compare April 5, 2018 00:27
src/test.ts Outdated
@@ -26,7 +26,7 @@ function compareToBaselines() {

function compileGeneratedFile(file: string) {
try {
child_process.execSync(`node ${tscPath} --strict --lib es5 --noEmit ${path.join(outputFolder, file)}`);
child_process.execSync(`node ${tscPath} --strict --lib es5 --skipLibCheck --noEmit ${path.join(outputFolder, file)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

why? we want to make sure we are generating valid declaration file.. that is the whole point of the test.

Copy link
Contributor Author

@saschanaz saschanaz Apr 5, 2018

Choose a reason for hiding this comment

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

This breaks @types/jsdom which depends on DOM types. I think we are still testing the input library file, this just turns off checks for JavaScript types.

Edit: @types/jsdom only breaks when checking worker types, as it lacks DOM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then Add ‘—types’ to exclude all @types packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but how can I pass empty string to --types? --types [], --types "", or --types "''" don't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

just --types..

--strict --lib es5 --types --noEmit

@mhegazy mhegazy merged commit 65fceb8 into microsoft:master Apr 5, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Apr 5, 2018

thanks!

@saschanaz saschanaz deleted the widl branch April 6, 2018 11:12
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.

2 participants