-
Notifications
You must be signed in to change notification settings - Fork 357
Promisified return value #205
base: gh-pages
Are you sure you want to change the base?
Conversation
@@ -19,6 +19,7 @@ | |||
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