Promise support

[yuvke]

Now that Promises are supported natively for a while now in most modern browsers, it would be great if loadjs would also support the following syntax:

loadjs(['foo.js', 'bar.js'])
    .then(function() { /* foo.js & bar.js loaded */ })
    .catch(function(depsNotFound) { /* either foo.js or bar.js load failed */ });

To do that, loadjs now returns a promise that resolves if all scripts are loaded successfully and rejects if at least one fails to load. A promise will only be returned if promises are available in the current environment and if no other arguments besides the paths were passed (doesn't support bundles).

[amorey]

Thanks for the PR! Unfortunately, promises aren't supported in IE11 but it's still at 2.8% usage which might be important for some developers (especially those targeting corporate environments). How would you advise developers who want to support older browsers to use this code in a cross-browser way?

[yuvke]

Well, developers that run their code in an environment that doesn't support promises can still enjoy all the other methods of using loadjs. If you're developing and you know you need to support IE, then you know that you don't have native promises. It can also be emphasized on the docs that loadJS will only return a promise in an environment that supports promises. This leaves the developer the option to bring in a polyfill if this is something that's important to him.

[amorey]

If I wanted to target both IE and Chrome would I have to handle both conditions?

var deps = ['foo.js', 'bar.js'],
    successFn = function() { /* foo.js & bar.js loaded */ },
    errorFn = function(depsNotFound) { /* either foo.js or bar.js load failed */ };

if (window.Promise) loadjs(deps).then(successFn).catch(errorFn);
else loadjs(deps, {success: successFn, error: errorFn});

[yuvke]

That's one way of handling it. Another way would be to use something like es6-promise and which only loads if native promises are not available and then always use the promise syntax.

Or yet a third option - you can decide to use just the callback syntax and forgo promises altogether.

I'm guessing that for most users - promises are used everywhere (not to mention async await) and if they need they use either polyfills or code transpiling.

[amorey]

Would it be possible to return a promise AND handle traditional callbacks? That way developers would have the flexibility of choosing which one to use rather than letting the library choose for them.

On May 30, 2018, at 8:36 AM, yuvke notifications@github.com wrote:

That's one way of handling it. Another way would be to use something like es6-promise and which only loads if native promises are not available and then always use the promise syntax.

Or yet a third option - you can decide to use just the callback syntax and forgo promises altogether.

I'm guessing that for most users - promises are used everywhere (not to mention async await) and if they need they use either polyfills or code transpiling.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[yuvke]

Hey @amorey, it does exactly that. There are currently two conditions for it to return a promise:

1. Promises are supported
2. No other arguments are passed So if the developer passes a callback, it will not return a promise. So if you choose to pass a callback argument it will not return a promise.

I didn't see much sense in returning a promise, in case the user passed a callback, because then you would need to decide, when is the promise resolved - before or after the callback is called? Neither approach makes more sense than the other. Essentially, promises are callbacks in a different syntax, so if you passed a callback argument, you're most likely not using promises.

[amorey]

Ok, I'd like to keep thinking about it but my first reaction is that the module would be more flexible and more predictable if we remove condition 2. I'd execute callback arguments first since this is consistent with the behavior of the .ready() method:

loadjs(['foo.js', 'bar.js'], 'foobar', {success: successFn1, error: errorFn1});

loadjs.ready('foobar', {success: successFn2, error: errorFn2});

Should .ready() return a promise as well?

[yuvke]

So would you imagine something like this?

loadjs(['foo.js', 'bar.js'], 'foobar',
    {
      success: function() { console.log('1st success!'); },
      error: function() { console.log('1st failure'); }
    }
  ).then(function() {
    console.log('3rd success');
  }).catch(function() {
    console.log('3rd failure');
  });

loadjs.ready('foobar', 
    {
      success: function() { console.log('2nd success!'); },
      error: function() { console.log('2nd failure'); }
    }
  ).then(function() {
    console.log('4th success');
  }).catch(function() {
    console.log('4th failure');
  });

So promises are always resolved after callbacks are called (both 'regular' and 'publish' callbacks).

[amorey]

Yes, that looks good to me. What do you think?

[yuvke]

It took me a while to get used to it, but after I looked at the .get jQuery method docs, it looks ok to me too.

Regarding .ready(), at first I thought it should return a promise as well, but now I see that in fact you can chain .ready() calls. Returning a promise would mean to remove that ability (which is a breaking change), so I think that it's probably best to keep it as-is no?

[yuvke]

Hi @amorey, I've given it some thought over the weekend, and it seems there's no easy way of consolidating the new promise behaviour with the old callbacks methods.

The main reason is that if you reject a promise, and that rejection isn't caught then in some environments (chrome and node) an unhandledrejection event will be thrown. So for everyone who's currently using the library on chrome they'll start seeing errors in their console everytime a script isn't loaded. Which would probably be a weird experience for them.

So I think I'm going on a different approach. I'm going to write a library called loadjs-promise which just wraps loadjs. So people who want to use loadjs with promises will just use that.

Should you want to upgrade loadjs to promises at some point I would advise doing this in a Major version thus informing the users that this is a big change.

[amorey]

Ok, that makes sense. Hopefully we can add promises to a later version of LoadJS.

[amorey]

@yuvke I was thinking about Promises recently. Did we consider adding promises via a separate function? For example:

loadjs(['foo.js', 'bar.js']).promise()
  .then(function() { /* foo.js & bar.js loaded */ })
  .catch(function(depsNotFound) { /* either foo.js or bar.js load failed */ });

Making the instantiation of a promise explicit would alleviate some of the issues we discussed earlier.

[XhmikosR]

IMO, it'd be better to provide a version which targets modern browsers and thus they have Promises support and make it a major version bump. You could keep the old branch around for bugfixes for some time if you want.

Just my 2 cents.

[amorey]

@XhmikosR Thanks for the feedback. Using a major version bump to offer a Promises-only interface makes sense but do you see a drawback to offering promises as an option to the existing library?

[XhmikosR]

The increased size, which IIRC you care about it a lot :)

[yuvke]

I think there are two separate questions here:

API question What should the API be to getting a promise? If it's explicit - calling .promise() - then it's backwards compatible. If it's implicit - returning a promise by default - then it's a breaking change, and it raises a second question.

Backwards Compatibility One way is to announce a major bump like @XhmikosR says and have the current version function as-is. Another option, if you still want to offer promise to the existing library, is to add some promise library to the current version (which of course would increase its size, and exclude it from the major bump.

My 2cents would be the same as @XhmikosR. Make the default return value a promise and bump the version so it won't break anyone's existing code. To me however, the API question is a design / aesthetic question, and this is just my preference. I do see also sense to having the API explicit (I would however prefer a returnPromise: true configuration option and not an extra function).

[amorey]

@XhmikosR Yes, file size is what keeps me up at night. It's death by a thousand cuts it's just hard to know which cuts help and which ones hurt.

@yuvke Thanks for the feedback. A returnPromise option is a good idea and would be compatible with the existing API. I think I'll take your code and modify it to see how many bytes it will add to the library.

[amorey]

Here's a new PR that adds the functionality we discussed earlier. It also supports success/error callbacks and loadjs.ready() callbacks simultaneously. The increase in byte size is 38 bytes. Let me know what you think: https://github.com/muicss/loadjs/pull/86

[amorey]

@yuvke @XhmikosR Thanks again for the feedback! I added support for Promises to v3.6.0 via a returnPromise option. Please try it out and let me know what you think.