Fix image loading issue in v5 of lottie-web #14
Conversation
There was a problem hiding this comment.
Thank you very much for this PR. I haven't tested myself, but if it fixes the image loading issue that's awesome for lottie-node, and makes it a viable option again 👍. I haven't used lottie-node myself for years, so I haven't kept track of upstream changes. It seems Lottie-web broke compatibility with lottie-node at some point, by refactoring. That was not unexpected, given lottie-node is pretty much a hack. I think I even warned about that in the readme.
I'll be happy to merge this, but first need some changes or answers.
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "lottie-node", | |||
| "version": "1.0.0", | |||
| "version": "1.1.0", | |||
There was a problem hiding this comment.
Never change this in a pull request. This is for the maintainer to decide. It's also a breaking change (not backward compatible), so the major number should be changed (v2, not v1.1.0)
| const {JSDOM} = require('jsdom'); | ||
| const {Canvas} = require('canvas'); | ||
| const { JSDOM } = require('jsdom'); | ||
| const { Canvas } = require('canvas'); |
There was a problem hiding this comment.
This change and also row 30, 35, 58 and 60 are formatting only, and you shouldn't do that when contributing to other people code. At least ask first. It also makes your PR harder to merge if people make formatting changes, especially if many people do.
If you do want to propose cleaning up/improving the formatting (I don't mind), that should be a separate PR, or at least separate commit.
| const {window} = new JSDOM('', {pretendToBeVisual: true}); | ||
| const {document, navigator} = window; | ||
| const {Image} = Canvas; | ||
| const { window } = new JSDOM('', { pretendToBeVisual: true, "resources": "usable" }); |
There was a problem hiding this comment.
"resources": "usable" sounds like a good option 👍. I'm not sure if it's new or I missed it before.
| assetData: assetData, | ||
| }; | ||
|
|
||
| require('canvas').loadImage(path).then((image) => { |
There was a problem hiding this comment.
Canvas is already required, and if you need to use loadImage you can add that to line 4 instead const { Canvas, loadImage } = require('canvas');
Require should always be unconditional and on top if possible "Since require() does a synchronous load, it can cause performance problems when used in other locations." -eslint rule
| const createContent = ` | ||
| function createImgData(assetData) { | ||
| var path = getAssetsPath(assetData, this.assetsPath, this.path); | ||
| var ob = { |
There was a problem hiding this comment.
What does ob mean?
Please see: https://medium.com/coding-skills/clean-code-101-meaningful-names-and-functions-bf450456d90c
index.js
Outdated
| ob.img = image; | ||
| this._imageLoaded(); | ||
| }).catch((err) => { | ||
| ob.img = proxyImage; |
There was a problem hiding this comment.
What is proxyImage? It's not defined anywhere.
| this._imageLoaded(); | ||
| }); | ||
|
|
||
| return ob; |
There was a problem hiding this comment.
This returns ob before the image is loaded and assigned to ob.img. Is this safe?
|
Rather than accepting this PR which comes with too many changes I don't want I cherry-picked the new image loading method in 6cb9be7 Thanks for the fix! This PR and that commit doesn't actually fix #8. This fixes Lottie breaking compatibility with our image loading code injection, which is more serious I guess. I didn't need |
Fixes issue #8