7
\$\begingroup\$

I wrote a small function that checks if an image or multiple images are loaded. For that purpose, I decided to use ES6 promises, but I'm not really sure if my way of handling errors is the best way.

const loadImg = function loadImg(src) {
    'use strict';

    const paths = Array.isArray(src) ? src : [src];
    const promise = [];

    paths.forEach((path) => {
        promise.push(new Promise((resolve, reject) => {
            const img = new Image();

            img.onload = () => {
                resolve({
                    path,
                    status: 'ok',
                });
            };

            // Call `resolve` even if the image fails to load. If we were to
            // call `reject`, the whole "system" would break
            img.onerror = () => {
                resolve({
                    path,
                    status: 'error',
                });
            };

            img.src = path;
        }));
    });

    return Promise.all(promise);
};

// Usage
loadImg([
    'IMG.png',
    'IMG.jpg',
    'IMG.jpg',
    'IMG.jpg',
]).then(function(e) {
    console.log(e);
});

It works, but I'm worried that using objects as error messages rather than reject has some disadvantages that I'm not aware of (e.g. potential memory leaks or people don't expect to see errors in then())? Is there any way I can improve this function?

\$\endgroup\$
  • 1
    \$\begingroup\$ Why did you opt to not reject() on error? \$\endgroup\$ – Madara Uchiha May 17 '16 at 20:17
  • \$\begingroup\$ @MadaraUchiha If I did, the Promise would fail even if only one image in many, say 20, didn't load for some reason. Secondly, if two images out of 10 fail, there is (?) no way to display which of them failed. I can only show one path. So the error messages were kind of useless too. That's why I opted out, but I'm open to suggestions if there's a better way to solve this. \$\endgroup\$ – demrks May 17 '16 at 20:31
  • \$\begingroup\$ That's fair. I can see the point. \$\endgroup\$ – Madara Uchiha May 17 '16 at 20:34
19
\$\begingroup\$

Well, you normally don't want to use the Promise constructor in your higher level code, you want to Promisify as low as possible. So let's create a function that checks for a single image, and resolves whenever you know the status of that image:

// When there's only one statement, you can drop the {} and the return
// x => y is equivalent to x => { return y; }
const checkImage = path =>
    new Promise(resolve => {
        const img = new Image();
        img.onload = () => resolve({path, status: 'ok'});
        img.onerror = () => resolve({path, status: 'error'});

        img.src = path;
    });

Now, with that Promise returning action, we can do some fancy things:

const loadImg = paths => Promise.all(paths.map(checkImage))

Or, if you want to get fancier

const loadImg = (...paths) => Promise.all(paths.map(checkImage));
// and call with
loadImg('path1', 'path2', 'path3');
// or
loadImg(...arrayOfPaths)

It's perfectly fine to not reject if there's no error. Yes, there was an error loading the image, but it isn't an exceptional situation, it's not something you'd throw because of. Your function is actually there to check if the image loaded fine or not.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.