Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

Promisified return value #205

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from
Open

Conversation

AABoyles
Copy link

@AABoyles AABoyles commented Feb 6, 2019

Resolves #204

@@ -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;
Copy link
Owner

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?

Copy link
Author

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);
Copy link
Owner

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?

Copy link
Author

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:

Suggested change
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 => {
Copy link
Owner

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.

Copy link
Author

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 => {
Copy link
Owner

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.

Copy link
Author

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.

@exupero
Copy link
Owner

exupero commented Feb 8, 2019

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));
   };
 })();

download appears to be synchronous, so the promise should only complete once the link has been clicked and removed.

@AABoyles
Copy link
Author

AABoyles commented Feb 8, 2019

That looks great! Thanks for working with me on this!

@exupero
Copy link
Owner

exupero commented Feb 10, 2019

I just published version 1.4.9 with these changes. Let me know if they work for you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callback or Promise on complete
2 participants