image: Fix options.imagebitmap on Firefox #616
Conversation
| } | ||
|
|
||
| function isEmptyObject(object) { | ||
| for (const key in object) { |
There was a problem hiding this comment.
This doesn’t seem to account for object===undefined
There was a problem hiding this comment.
This doesn’t seem to account for object===undefined
Shouldn't happen due to the way options are merged with defaults that said adding the check doesn't cost much., will do.
There was a problem hiding this comment.
Looking at the previous attempt: d348845
Does it throw immediately or does the promise reject?
There was a problem hiding this comment.
Does it throw immediately or does the promise reject?
Yes I believe you have a point. async await converts automatically between promise rejections and exceptions, so I'll update to use async syntax.
Still this makes it obvious that I have not been able to test these changes, due to our test harness limitations.
There was a problem hiding this comment.
Could you hold on merging while I fix the testing?
Ye we can make the dependency optional. Render tests won’t work because the screen capture and comparison is done on the server side. Puppeteer has experimental support for Firefox but I have never tried. |
That's cool, it would be great if we could have dual browser test automation, though I'd prioritize the puppeteer-less mode so that we can run unit tests in Safari and other browsers too. |
|
|
Image tests are passing on Firefox (however the new setup uncovered two stream related errors which is great). Will follow up on those separately. |
As reported by @isaacbrodsky, Firefox crashes if options are passed to
createImageBitmap.This fix chooses to still support
options.imagebitmapon browsers that do support that parameter.@Pessimistress While trying to test this fix, I realized that the page served by
yarn test browseris dependent on puppeteer and can't be run in other browsers. Would be nice to have the option to easily run our unit tests in other browsers. Not sure if we could just make the puppeteer calls optional if not run on Chromium, or if we'd need a separate ocular sub target.