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

lib from W3C/WHATWG #227

Closed
wants to merge 50 commits into from
Closed

Conversation

saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Apr 2, 2017

Fixes #101.

The input XML in this PR is generated by webidl-serializer.

The changes in TS.fsx are to:

  1. support new namespace types (67bcdd6)
  2. support multiple constructor signatures (61070d8)
  3. add parentheses on variadic union param types -> someMethod(args: ...(Node | string)[]) (
    e105830)
  4. add missing IDL primitive type mappings
  5. ensure that member conflict solver won't affect other unlisted interfaces (f1c26da)
  6. use Flavor.Web (8875852)
  7. filter by Exposed instead of Tags (853c7f6)
  8. replace webworker.specidl.xml by browser.webidl.xml (103c39e)

Current remaining problems:

  • Note: No SVG 1.1 spec as it is written with OMG IDL which is not compatible with Web IDL. (and probably not type-compatible with SVG 2)

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 23, 2017

This PR contains several types from Working Draft and Editor's Draft to be compatible with current lib.d.ts where pre-CR specs e.g. Web Audio, Touch Events Level 2, and Intersection Observer are already included.
Full list: https://github.com/SaschaNaz/webidl-serializer/blob/master/specs.json

@saschanaz saschanaz changed the title Intent to merge: lib from W3C/WHATWG lib from W3C/WHATWG Apr 23, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Apr 26, 2017

@saschanaz sorry for the delay. i am not sure what is the best way to move on this. it is a bit hard to see the impact of the change due to the size of the change. i wounder if there is a way we make this more managable..

for instance, can we change the current generated file to first have the same order as the new file, the do the change, at least we can limit the diff noise..

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 27, 2017

I'm not sure there is a good way to reorder current generated file to match this PR, since there is no rule for the order. Instead I can reorder new file by sorting them alphabetically. (If we don't want the sorting then we can open a new PR to undo it)

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2017

I just need a way to see the changes and asses the differences. We can do changes to the existing repo to reorder first, then you can use the same order well, this way we can compare.

@saschanaz
Copy link
Contributor Author

Yes, I mean the existing one is already sorted (though callback-functions are not) so better to fix this PR to match it. The last commit is not sufficient, currently I'm testing locally to make seeing diff easy.

@saschanaz
Copy link
Contributor Author

Yes, I mean the existing one is already sorted

Not true, some are sorted and others are not. Will push another PR to sort them.

@saschanaz saschanaz force-pushed the constructors branch 6 times, most recently from 777b388 to b7731e8 Compare May 1, 2017 16:27
@saschanaz saschanaz force-pushed the constructors branch 5 times, most recently from 61a13ff to f06d702 Compare May 19, 2017 15:53
@saschanaz
Copy link
Contributor Author

I think now it's ready to get some review as I don't see any major/minor problems on code diff itself. If you see some problems please tell me 😉

@falsandtru
Copy link
Contributor

ping @mhegazy

@saschanaz saschanaz force-pushed the constructors branch 2 times, most recently from 12e1e8f to c17b984 Compare May 27, 2017 08:21
@falsandtru
Copy link
Contributor

@mhegazy Are you reviewing this now???
cc @sandersn @DanielRosenwasser

@saschanaz
Copy link
Contributor Author

It seems reviewing this PR is too time-consuming because of the diff size. A progressive approach would be great but I'm not sure how as of now.

@saschanaz
Copy link
Contributor Author

My current thought: Do not replace XML side, instead add an addedTypes.json-like progressive data.

@NaridaL
Copy link

NaridaL commented Sep 12, 2017

I've looked at the dom.generated.d.ts... the order is nearly the same, but some things seem to have moved, e.g. ScrollOptions has moved up 11k lines.

An issue is that the spec source doesn't seem to be quite as detailed in some respects. For example, the static members on WebGlContext are missing. (tbh, I didn't find them in the spec, but all the browsers have them...). Also, some constructors are missing, e.g. for SpeechSynthesisEvent

A lot of interfaces are missing, but it looks like they are mostly Microsoft specific things. Maybe a first step would be to add those types to the removedTypes.json to see if it breaks anything. Note that some types have just been renamed slightly, so it may be necessary to check if the type is being used elsewhere in the file before removing it.

@saschanaz
Copy link
Contributor Author

saschanaz commented Sep 16, 2017

@NaridaL Thank you for your review 👍

e.g. ScrollOptions has moved up 11k lines.

Yeah, addedTypes.json adds its types on different position, so this movement occurs.

For example, the static members on WebGlContext are missing. (tbh, I didn't find them in the spec, but all the browsers have them...).

I'm not sure what you mean, I couldn't find an interface named WebGlContext.

Also, some constructors are missing, e.g. for SpeechSynthesisEvent

You're right, this is basically because the spec (which hasn't been updated for years) doesn't have it :( Someone filed a bug about this.

Maybe a first step would be to add those types to the removedTypes.json to see if it breaks anything.

Yes, probably a separate PR to remove the MS-specific things is the right way to do, and then the next thing is moving them to DefinitelyTyped 😄

@NaridaL
Copy link

NaridaL commented Sep 16, 2017

@saschanaz WebGLRenderingContext, sorry. All the constants are available on the global WebGLRenderingContext in browsers. (And in the current generated.d.ts)

@saschanaz
Copy link
Contributor Author

Ah, now I understand. They are defined in WebGLRenderingContextBase which WebGLRenderingContext depends on.

@NaridaL
Copy link

NaridaL commented Sep 16, 2017

@saschanaz The interface is fine, but they're missing on the declared variable. https://github.com/Microsoft/TSJS-lib-generator/pull/227/files#diff-a97a9d0dbf71ec2f96be57f7fb9f70c9L12517

@saschanaz
Copy link
Contributor Author

saschanaz commented Sep 16, 2017

Hmm, it seems that's because emitConstants currently only checks the child interface members. I didn't catch that. Thanks!

(Probably iNameToEhParents-like mapping will be required)

@saschanaz
Copy link
Contributor Author

Superceded by #300.

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.

6 participants