Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Modify tool to support processing multiple .d.ts files #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

staxmanade
Copy link

In preparation for work in winjs/react-winjs#31 this allows us to pass in multiple file names.

@staxmanade
Copy link
Author

Unfortunately it's reporting a different issue now:

Warning: Command failed: ./node_modules/winjs-control-apis/main.js ./typings/winjs.d.ts ./typings/MediaElement.d.ts > ./RawControlApis.js
/Users/jasonj/code/personal/winjs/winjs-control-apis/tscore.js:662
    throw new TypeError("Incompatible types: " + typ + " and " + other)
    ^

Error: Incompatible types: string and string (line 694)
    at new TypeError (.../winjs/winjs-control-apis/tscore.js:14:9)
    at mergePropertyInto (.../winjs/winjs-control-apis/tscore.js:662:8)
    at .../winjs/winjs-control-apis/tscore.js:773:7
    at Map.forEach (.../winjs/winjs-control-apis/lib/map.js:30:9)
    at mergeInto (../winjs/winjs-control-apis/tscore.js:767:22)
    at .../winjs/winjs-control-apis/tscore.js:796:17
    at Map.forEach (.../winjs/winjs-control-apis/lib/map.js:30:9)
    at mergeObjectTypes (.../winjs/winjs-control-apis/tscore.js:793:17)
    at .../winjs/winjs-control-apis/tscore.js:791:7
    at Map.forEach (.../winjs/winjs-control-apis/lib/map.js:30:9)
 Use --force to continue.

/cc @rigdern

@staxmanade
Copy link
Author

If I try to run just the MediaElement.d.ts file through the tool I get:

Unknown capitalization for some events.
Warning: Command failed: ./node_modules/winjs-control-apis/main.js ./typings/MediaElement.d.ts > ./RawControlApis.js
Unknown capitalization for the following events. Please update eventNameCapitalization to include these events:
  onafterhidecontrols: "onafterhidecontrols",
  onaftershowcontrols: "onaftershowcontrols",
  onbeforehidecontrols: "onbeforehidecontrols",
  onbeforeshowcontrols: "onbeforeshowcontrols",
  onmarkerreached: "onmarkerreached",
  onmediacommandexecuted: "onmediacommandexecuted",
  ontargetratechange: "ontargetratechange",
  ontargettimeupdate: "ontargettimeupdate",
  onthumbnailrequest: "onthumbnailrequest"

./winjs/winjs-control-apis/main.js:211
        throw "Unknown capitalization for some events.";
        ^
Unknown capitalization for some events.
 Use --force to continue.

@staxmanade
Copy link
Author

If I update the eventNameCapitalization with the following (made a bit of a guess on the right hand side as I didn't verify each one) it can generate the RawControlApis for the MediaElementAdapter and MediaPlayer.

I'm confused why the .d.ts has all lower case, but the actual code has camelCase

    onafterhidecontrols: "onAfterHideControls",
    onaftershowcontrols: "onAfterShowControls",
    onbeforehidecontrols: "onBeforeHideControls",
    onbeforeshowcontrols: "onBeforeShowControls",
    onmarkerreached: "onMarkerReached",
    onmediacommandexecuted: "onMediaCommandExecuted",
    ontargetratechange: "onTargetRateChange",
    ontargettimeupdate: "onTargetTimeUpdate",
    onthumbnailrequest: "onThumbnailRequest"

@rigdern
Copy link
Contributor

rigdern commented Oct 16, 2015

A few things:

  • Did you manage to fix that first error? throw new TypeError("Incompatible types: " + typ + " and " + other).
    • If not, here are some hints. winjs-control-apis utilizes tscore (which is part of a 3rd party library called tscheck) to parse the d.ts file. If for some reason tscore doesn't like you passing in multiple files which contribute to the same module (WinJS.UI), another approach would be to call into tscore multiple times, once per input file.
  • It looks like you haven't pushed an update containing the changes to eventNameCapitalization. Can you update your pull request with those changes?
  • It looks like you haven't yet removed these lines so winjs-control-apis will output the API surface for MediaPlayer. Can you include that in your change?
  • Minor request: while you're changing this code, can you change the console.logs in these lines to console.errors so that people will see those messages in their command prompt even if they pipe the output of winjs-control-apis into a file?

You asked:

I'm confused why the .d.ts has all lower case, but the actual code has camelCase

I'm not sure what you mean by "actual code" but I'll do my best to answer your question. The convention for event names in JavaScript is for them to be all lowercase (e.g. onmouseover). The convention for props representing events in React is for their names to be camel cased (e.g. onMouseOver). So that's why WinJS and its d.ts use lowercased event names whereas react-winjs (with the assistance of winjs-control-apis) uses camel cased event names.

Just to double check, when you mention ./typings/MediaElement.d.ts, is that the same as the mediaplayer.d.ts gist I posted?

@staxmanade
Copy link
Author

Hey @rigdern

Did you manage to fix that first error? throw new TypeError("Incompatible types: " + typ + " and " + other).

I tried looking into it - but didn't get far enough to understand what's going on... I'll give your suggestion a try.

It looks like you haven't pushed an update containing the changes to eventNameCapitalization. Can you update your pull request with those changes?

Published now. Let me know if you notice any issues...

It looks like you haven't yet removed these lines so winjs-control-apis will output the API surface for MediaPlayer. Can you include that in your change?

Commented out the media element from the ignore list.

Minor request: while you're changing this code, can you change the console.logs in these lines to console.errors so that people will see those messages in their command prompt even if they pipe the output of winjs-control-apis into a file?

No problem above (I did do that in a couple places) - do you want them everywhere or only specific locations changes? (I don't think I added any logs so I'm not sure which ones you're referring to).

Just to double check, when you mention ./typings/MediaElement.d.ts, is that the same as the mediaplayer.d.ts gist I posted?

I did use the gist you published. Thanks for that.

NOTE: I'll have a PR for the react-winjs project here in a bit that automates the call into winjs-control-apis. Since I didn't get it working all the way through I haven't published it yet. But I'll get an early commit PR setup for your review.

"WinJS.UI.MediaElementAdapter",
"WinJS.UI.MediaPlayer",
// "WinJS.UI.MediaElementAdapter",
// "WinJS.UI.MediaPlayer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you delete those 2 lines instead of commenting them out?

@rigdern
Copy link
Contributor

rigdern commented Oct 16, 2015

I left some feedback on the code in your PR which you can see by viewing the "Files changed" tab.

Minor request: while you're changing this code, can you change the console.logs in these lines to console.errors so that people will see those messages in their command prompt even if they pipe the output of winjs-control-apis into a file?

No problem above (I did do that in a couple places) - do you want them everywhere or only specific locations changes? (I don't think I added any logs so I'm not sure which ones you're referring to).

You didn't add any console.logs. While reviewing your change, I happened to notice a couple of console.logs that were error messages and would be better written as console.errors. You updated exactly the ones I wanted.

Keep up the good work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants