-
Notifications
You must be signed in to change notification settings - Fork 4
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
Implement Node support #5
Conversation
Great work! I'll have a closer look later this week, but one thing that bothers me is that we have to turn the images into data-URL strings, which has a high overhead (~33% data size increase and time to encode/decode). Ideally the API would take and return one of the following:
the least one would be my preference. some things to look at - maybe that will help solve this issue:
we could also use the above libraries instead of canvas - not sure. |
also a tiny minor request - could you make sure all the indentation is consistent across the code? 😇 but yeah I know auto-format from editors can be a pain 😅 |
ah - I just realised now that the only reason we'd need a promise-based API is if we're loading remote images. so yeah sorry, I got myself a bit confused! but basically - can we split this into multiple functions? It would be good to have a func that doesn't do promises, but rather just the pure transformation and then we provide a wrapper that adds image loading via promises as a utility. sorry for causing confusion 😄 |
e9836e4
to
3bfdabf
Compare
Thanks for the feedback, @markus-wa. 🙂 I've re-thought it a bit, and realised it's just as straightforward for the image loading to be done before passing the image to Stickerify: this removes the API breakage that I'd originally proposed.
No worries, I think I've fixed it all up, but please double check it. If you happen to have a linting config you could add to the project, that'd be super helpful. 🙂 |
3bfdabf
to
55f233a
Compare
Thanks @pento - that looks and sounds all good to me! I will add a linting config later this week and make sure all the code is compliant with that 🙂 But for now I'll merge it as is! |
just published v0.2.0 with this change 🚀 - thanks again! |
Demo: https://codepen.io/pento/pen/OJjZPgE
Fixes #4.
Uses the
canvas
package, which provides a Node implementation of<canvas>
, as well as some basicImage
support (sufficient for these purposes, at least).When running in the browser, it provides a small shim that maps to
document.createElement('canvas')
anddocument.createElement('img')
.Where Stickerify currently takes a
HTMLImageElement
and returns aHTMLCanvasElement
, running it in Node.js will cause it to take anImage
object, and return aCanvas
object, both defined by thecanvas
package.Here's an example of Node usage (I've also added this to the README):