-
Notifications
You must be signed in to change notification settings - Fork 359
Promisified return value #205
base: gh-pages
Are you sure you want to change the base?
Conversation
| const isElement = obj => obj instanceof HTMLElement || obj instanceof SVGElement; | ||
| const requireDomNode = el => { | ||
| if (!isElement(el)) throw new Error(`an HTMLElement or SVGElement is required; got ${el}`); | ||
| return; |
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.
Why are these return statements necessary?
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.
They aren't, just experiment cruft. Sorry about that!
| requireDomNode(el); | ||
| out$.svgAsDataUri(el, options || {}, uri => out$.download(name, uri)); | ||
| return new Promise((resolve, reject) => { | ||
| requireDomNode(el); |
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.
Can requireDomNode(el) be left outside the promise?
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.
Probably, but I think that would be a step in the wrong direction. It looks like requireDomNode is basically just to halt execution if el isn't a DOM node. If we're going to throw an error when we're returning a promise, best practice would be to call reject instead. Something like this:
| requireDomNode(el); | |
| return new Promise((resolve, reject) => { | |
| if(isDomNode(el)){ | |
| resolve(); | |
| } else { | |
| reject(); | |
| } | |
| }); |
| out$.svgAsDataUri(el, options || {}, uri => out$.download(name, uri)); | ||
| return new Promise((resolve, reject) => { | ||
| requireDomNode(el); | ||
| out$.svgAsDataUri(el, options || {}, uri => { |
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.
I think svgAsDataUri already returns a promise.
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.
Yes, but download does not, and I need to make sure that my callback fires after the entire stack is called.
| out$.svgAsPngUri(el, options || {}, uri => out$.download(name, uri)); | ||
| return new Promise((resolve, reject) => { | ||
| requireDomNode(el); | ||
| out$.svgAsPngUri(el, options || {}, uri => { |
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.
I think svgAsPngUri already returns a promise.
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.
Yes, but download does not, and I need to make sure that my callback fires after the entire stack is called.
|
Thanks for the feedback. Does something like this work? diff --git a/src/saveSvgAsPng.js b/src/saveSvgAsPng.js
index 620af07..30cc81c 100644
--- a/src/saveSvgAsPng.js
+++ b/src/saveSvgAsPng.js
@@ -20,6 +20,11 @@
const requireDomNode = el => {
if (!isElement(el)) throw new Error(`an HTMLElement or SVGElement is required; got ${el}`);
};
+ const requireDomNodePromise = el =>
+ new Promise((resolve, reject) => {
+ if (isElement(el)) resolve(el)
+ else reject(new Error(`an HTMLElement or SVGElement is required; got ${el}`));
+ })
const isExternal = url => url && url.lastIndexOf('http',0) === 0 && url.lastIndexOf(window.location.host) === -1;
const getFontMimeTypeFromUrl = fontUrl => {
@@ -367,12 +372,14 @@
};
out$.saveSvg = (el, name, options) => {
- requireDomNode(el);
- out$.svgAsDataUri(el, options || {}, uri => out$.download(name, uri));
+ requireDomNodePromise(el)
+ .then(out$.svgAsDataUri(el, options || {}))
+ .then(uri => out$.download(name, uri));
};
out$.saveSvgAsPng = (el, name, options) => {
- requireDomNode(el);
- out$.svgAsPngUri(el, options || {}, uri => out$.download(name, uri));
+ requireDomNodePromise(el)
+ .then(out$.svgAsPngUri(el, options || {}))
+ .then(uri => out$.download(name, uri));
};
})();
|
|
That looks great! Thanks for working with me on this! |
|
I just published version 1.4.9 with these changes. Let me know if they work for you. |
Resolves #204