-
-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Promise support #68
Promise support #68
Conversation
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 beside the paths were passed (doesn't support bundles)
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? |
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. |
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}); |
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. |
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 ***@***.***> 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.
|
Hey @amorey, it does exactly that. There are currently two conditions for it to 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. |
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 loadjs(['foo.js', 'bar.js'], 'foobar', {success: successFn1, error: errorFn1});
loadjs.ready('foobar', {success: successFn2, error: errorFn2}); Should |
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). |
Yes, that looks good to me. What do you think? |
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 |
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 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. |
Ok, that makes sense. Hopefully we can add promises to a later version of LoadJS. |
@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. |
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. |
@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? |
The increased size, which IIRC you care about it a lot :) |
I think there are two separate questions here: API question Backwards Compatibility 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 |
@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 |
Here's a new PR that adds the functionality we discussed earlier. It also supports success/error callbacks and |
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:
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).