Cleanup of P5Util class #1319
Cleanup of P5Util class #1319
Conversation
| // Ensure the p5Color object is an RGB array | ||
| Object.keys(result).forEach(part => { | ||
| if (result[part].color instanceof window.p5.Color) { | ||
| if (result[part].color instanceof p5.Color) { |
There was a problem hiding this comment.
positive affirmation:
- nice one! 🥇
| @@ -5,21 +5,31 @@ | |||
|
|
|||
| class P5Util { | |||
| constructor() { | |||
| if (typeof window !== "undefined") { | |||
There was a problem hiding this comment.
positive affirmation:
- huzzah! 🙌
| this.p5Instance.loadImage(url, (img) => { | ||
| resolve(img); | ||
| }, () => { | ||
| reject(new Error(`Could not load image from url ${url}`)); |
There was a problem hiding this comment.
positive affirmation:
- Yesss! Thanks so much for digging into the error handling here <3 🏅
| * @param {number} height | ||
| * @returns {Promise<Blob>} | ||
| */ | ||
| async rawToBlob(raws, width, height) { |
There was a problem hiding this comment.
positive affirmation:
- this is so helpful and makes much more sense to use width & height 🙌
There was a problem hiding this comment.
LGTM! Again, I'm loving the clarity you're bringing into both the code (jsdoc updates will help so much w/ introducing Types) and downstream end-user experience (e.g. adding error handling!). I'm excited about how these changes are moving us closer to being able to support ml5 in a node context! 🚀
| if (this.checkP5()) { | ||
| const p5Img = await this.loadAsync(URL.createObjectURL(blob)); | ||
| return p5Img; | ||
| if (this.checkP5() && typeof URL !== "undefined") { |
There was a problem hiding this comment.
Hello! I'm just reviewing code for fun. Curious – can you not just say if (this.checkP5() && URL) {...? Is it necessary to use typeof in order to check that it's not undefined?
There was a problem hiding this comment.
Great question! That particular check is part of a larger effort towards making sure that ml5 can run in a backend Node.js environment without error. Here I am making sure that the global URL constructor exists before I try to use it.
If a variable such as URL is never declared anywhere (with let, const, var, class, etc.) then trying to access if (URL) is a fatal ReferenceError. But checking typeof URL !== "undefined" is ok and lets you know that it's safe to use that variable. We definitely have to do this when accessing window.
It might be that's it's not actually needed for URL because Node does have its own URL class, which I didn't know until right now 😄
There was a problem hiding this comment.
Interesting! That makes sense to me. Appreciate the response – I've learned something new!
getBlobandloadAsyncwhen the underlying functions fail.xandytowidthandheightfor clarity in methodrawToBlob.This PR extends the commit in #1318 to avoid creating conflicts in the P5Util constructor.