Fix missing optional parameter of createImageBitmap #571

hexrcs · 2018-10-02T12:46:26Z

ImageBitmapOptions was declared both in addedTypes.json as an interface and in idl/HTML - ImageBitmap and animations.widl as a dictionary, which is I believe the root cause of it not showing up as an optional parameter inside createImageBitmap method. I removed the interface declaration.
Window and Worker have different supports for ImageBitmapSource (see this conversation). I created two distinct types ImageBitmapSourceWindow and ImageBitmapSourceWorker for them in the widl files.

This is my first PR to a major open source project ;) Please kindly review the changes I made. Suggestions would be much appreciated!

Different for Window or WorkerGlobalScope

msftclas · 2018-10-02T12:46:38Z

All CLA requirements met.

sandersn

I would like to return the definition of imageBitmapOptions to using inline unions instead of aliases.

sandersn · 2018-10-25T23:47:40Z

baselines/dom.generated.d.ts

@@ -17647,6 +17650,7 @@ type CanvasTextBaseline = "top" | "hanging" | "middle" | "alphabetic" | "ideogra
 type ChannelCountMode = "max" | "clamped-max" | "explicit";
 type ChannelInterpretation = "speakers" | "discrete";
 type ClientTypes = "window" | "worker" | "sharedworker" | "all";
+type ColorSpaceConversion = "none" | "default";


Small global aliases are annoying because of alias interning: everybody with a type none | default will now see the name ColorSpaceConversion for it.

Not sure the best workaround for this. I’ll think about it.

Thank you for reviewing, I will find time to come back and reconsider about this PR soon. :)

sandersn · 2018-10-25T23:51:02Z

inputfiles/addedTypes.json

@@ -396,45 +396,6 @@
                    }
                }
            },
-            "ImageBitmapOptions": {


What replaces this entry? Whatever it is, it results in a whole bunch of unwanted type aliases.

The already existing dictionary ImageBitmapOptions in HTML - ImageBitmap and animations.widl replaces this entry. This was a conflict if I remember right.

saschanaz · 2018-11-01T03:47:31Z

Are the IDL changes hand-written or generated by npm run fetch?

hexrcs · 2018-11-14T18:26:15Z

Are the IDL changes hand-written or generated by npm run fetch?

@saschanaz Sorry for the delay. They are handwritten. *.d.ts are generated.

saschanaz · 2019-01-25T01:09:21Z

Sorry to be late, but IDLs must not be hand-written and instead should be generated by npm run fetch -- (SpecName).

saschanaz · 2019-01-29T04:40:35Z

@hexrcs Mind refreshing this PR? The removal in addedTypes.json looks great but the manual edits in *.widl do not. Can you remove them and do npm run build && npm run baseline-accept?

Some explanation: The old item in addedTypes.json was incorrectly registered as an interface with no exposed field rather than a dictionary. This caused confusion and caused unexpected deletions as the code thought ImageBitmapOptions (and some typedefs referenced solely from it) shouldn't be exposed in any files.

hexrcs · 2019-01-29T08:38:41Z

@saschanaz Yes, would like to! Will be checking this out tonight or tomorrow. Thanks for the clarification.

hexrcs · 2019-01-31T10:12:40Z

@saschanaz Hi, which SpecName should I use to run npm run fetch -- (SpecName)? There doesn't seem to be any documentation about this. Thanks!

hexrcs · 2019-01-31T10:39:14Z

I tried to run npm run fetch -- "HTML - Web application APIs" after reading #628 (comment) , but the fetch script went on to update all the .widl files and ended up with an error.

Looks like something's wrong with WebGL2RenderingContext?

npm run fetch -- "HTML - Web application APIs"

> tsjs-lib-generator@ fetch /Volumes/SSD2/opensource/ms/TSJS-lib-generator
> echo This could take a few minutes... && npm run fetch-idl && npm run fetch-mdn "HTML - Web application APIs"

This could take a few minutes...

> tsjs-lib-generator@ fetch-idl /Volumes/SSD2/opensource/ms/TSJS-lib-generator
> npm run build && node ./lib/idlfetcher.js


> tsjs-lib-generator@ build /Volumes/SSD2/opensource/ms/TSJS-lib-generator
> tsc --p ./tsconfig.json && node ./lib/index.js


> tsjs-lib-generator@ fetch-mdn /Volumes/SSD2/opensource/ms/TSJS-lib-generator
> npm run build && node ./lib/mdnfetcher.js "HTML - Web application APIs"


> tsjs-lib-generator@ build /Volumes/SSD2/opensource/ms/TSJS-lib-generator
> tsc --p ./tsconfig.json && node ./lib/index.js

/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:281
        throw new Error("Unknown DOM type: " + objDomType);
        ^

Error: Unknown DOM type: WebGL2RenderingContext
    at convertDomTypeToTsTypeSimple (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:281:15)
    at convertDomTypeToTsTypeWorker (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:221:28)
    at Array.map (<anonymous>)
    at convertDomTypeToTsTypeWorker (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:224:36)
    at convertDomTypeToTsType (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:215:22)
    at emitTypeDef (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:906:60)
    at Array.forEach (<anonymous>)
    at emitTypeDefs (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:910:37)
    at emit (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:946:9)
    at Object.emitWebIdl (/Volumes/SSD2/opensource/ms/TSJS-lib-generator/lib/emitter.js:136:70)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! tsjs-lib-generator@ build: `tsc --p ./tsconfig.json && node ./lib/index.js`
npm ERR! Exit status 1

saschanaz · 2019-01-31T10:39:14Z

Hi, which SpecName should I use to run npm run fetch -- (SpecName)?

"HTML - ImageBitmap and animations", but it can be skipped for this PR. Looks like npm run build && npm run baseline-accept will fix the test.

saschanaz · 2019-01-31T10:40:23Z

Ah right, the code has been updated and now it should be npm run fetch-idl (note the -idl). Sorry for the confusion 😅

saschanaz · 2019-01-31T10:43:34Z

Looks like something's wrong with WebGL2RenderingContext?

That should be fixed by #648, it's because other specs has started referencing it while the code doesn't know about it.

hexrcs · 2019-01-31T10:45:54Z

@saschanaz Done! Could you please review again? 😃

saschanaz

👍 but still needs an approval from @sandersn

benjamind · 2019-10-09T16:21:25Z

Any progress or guidance on how to use createImageBitmap with options at this time?

hexrcs added 6 commits October 2, 2018 13:00

Add ImageBitmapSource types

b2fb7c7

Different for Window or WorkerGlobalScope

Modify widl for ImageBitmap

96eae1c

Remove conflicting defs in json

da508fb

Fix typo

479a646

Remove confliting ImageBitmapOptions in json

aea3f34

Update generated baseline files

7315100

hexrcs mentioned this pull request Oct 2, 2018

Removed optional parameter from Window.createImageBitmap factory microsoft/TypeScript#27475

Closed

sandersn requested changes Oct 25, 2018

View reviewed changes

saschanaz mentioned this pull request Jan 29, 2019

Remove redundant added types #647

Merged

Revert widl files to unmodified state

7d23352

Merge remote-tracking branch 'upstream/master'

ad5e0ca

Update widl via fetch-idl script

a88d12f

Build and run baseline test

a45cbb0

saschanaz approved these changes Jan 31, 2019

View reviewed changes

sandersn approved these changes Feb 25, 2020

View reviewed changes

Merge master

b1d3fd1

sandersn merged commit c5ad7c3 into microsoft:master Feb 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing optional parameter of createImageBitmap #571

Fix missing optional parameter of createImageBitmap #571

hexrcs commented Oct 2, 2018 •

edited

Loading

msftclas commented Oct 2, 2018 •

edited

Loading

sandersn left a comment

sandersn Oct 25, 2018

hexrcs Oct 27, 2018

sandersn Oct 25, 2018

hexrcs Oct 27, 2018

saschanaz commented Nov 1, 2018

hexrcs commented Nov 14, 2018 •

edited

Loading

saschanaz commented Jan 25, 2019

saschanaz commented Jan 29, 2019

hexrcs commented Jan 29, 2019

hexrcs commented Jan 31, 2019

hexrcs commented Jan 31, 2019

saschanaz commented Jan 31, 2019

saschanaz commented Jan 31, 2019

saschanaz commented Jan 31, 2019

hexrcs commented Jan 31, 2019

saschanaz left a comment

benjamind commented Oct 9, 2019

Fix missing optional parameter of createImageBitmap #571

Fix missing optional parameter of createImageBitmap #571

Conversation

hexrcs commented Oct 2, 2018 • edited Loading

msftclas commented Oct 2, 2018 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

sandersn Oct 25, 2018

Choose a reason for hiding this comment

hexrcs Oct 27, 2018

Choose a reason for hiding this comment

sandersn Oct 25, 2018

Choose a reason for hiding this comment

hexrcs Oct 27, 2018

Choose a reason for hiding this comment

saschanaz commented Nov 1, 2018

hexrcs commented Nov 14, 2018 • edited Loading

saschanaz commented Jan 25, 2019

saschanaz commented Jan 29, 2019

hexrcs commented Jan 29, 2019

hexrcs commented Jan 31, 2019

hexrcs commented Jan 31, 2019

saschanaz commented Jan 31, 2019

saschanaz commented Jan 31, 2019

saschanaz commented Jan 31, 2019

hexrcs commented Jan 31, 2019

saschanaz left a comment

Choose a reason for hiding this comment

benjamind commented Oct 9, 2019

hexrcs commented Oct 2, 2018 •

edited

Loading

msftclas commented Oct 2, 2018 •

edited

Loading

hexrcs commented Nov 14, 2018 •

edited

Loading