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

Promisified return value #205

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/saveSvgAsPng.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

};
const isExternal = url => url && url.lastIndexOf('http',0) === 0 && url.lastIndexOf(window.location.host) === -1;

Expand Down Expand Up @@ -339,6 +340,7 @@
image.src = uri;
})
});
return;
};

out$.download = (name, uri) => {
Expand All @@ -365,15 +367,26 @@
window.open(uri, '_temp', 'menubar=no,toolbar=no,status=no');
}
}
return;
};

out$.saveSvg = (el, name, options) => {
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 => {
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$.download(name, uri)
resolve();
});
});
};

out$.saveSvgAsPng = (el, name, options) => {
requireDomNode(el);
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.

out$.download(name, uri);
resolve();
});
});
};
})();