-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add browser compatible API #929
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
Conversation
package.json
Outdated
@@ -29,6 +30,7 @@ | |||
"test-server": "node test/server.js" | |||
}, | |||
"dependencies": { | |||
"assert-rejects": "^0.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to be a devDependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
case 0: return new ImageData() | ||
case 1: return new ImageData(array) | ||
case 2: return new ImageData(array, width) | ||
default: return new ImageData(array, width, height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do cases 0 and 1 exist? MDN only documents new ImageData(arr, w, h)
and ImageData(w, h)
.
Also, would this be the same as return new ImageData(...arguments)
? EDIT: (Or what you have in index.js
, where you're just passing three args, and height
may be undefined.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0
and 1
doesn't exist, I added them to get the errors reported back to be accurate.
return new ImageData(...arguments)
would be perfect, but unfortunately Node.js 4.x doesn't support it... It's doing this since passing (10, 10, undefined)
doesn't do what you want it to in the browsers:
Not 100% sure that this is the best approach though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, clever :) and now I understand what your comment means.
@LinusU I started playing more with this and realized one thing to consider changing. This PR hid the |
Actually, a lot of stuff got hidden... > /* 2.0 */ Canvas.
[inherited from Object, snip]
Canvas.createCanvas Canvas.createImageData Canvas.loadImage Canvas.parseFont > /* 1.6 */ Canvas.
[inherited from Object, snip]
[inherited from Function, snip]
Canvas.Context2d Canvas.Image Canvas.ImageData Canvas.JPEGStream
Canvas.PNGStream Canvas.cairoVersion Canvas.prototype Canvas.version
// 2.0 pre-this PR also had Canvas.backends The constructors are useful at least for Sounds like that was the intent actually:
Edit 3: Plus exposing all the original constructors means that all of the examples and benchmarks won't need to be updated, and legacy code will continue to work :) |
This PR modifies the API to a few wrapper functions that are provided both for Node.js and the browser. Using a tool like browserify it should be easy to write code that works both on the backend and the frontend 🙌
ref: #833