Skip to content
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

Doesn't handle 404 errors #31

Closed
apaatsio opened this issue Apr 13, 2012 · 13 comments
Closed

Doesn't handle 404 errors #31

apaatsio opened this issue Apr 13, 2012 · 13 comments
Labels

Comments

@apaatsio
Copy link

If file is not found basket.js saves the 404 message in localStorage. Try something like

basket.require('doesntexist123.js');

I would assume the same problem exists with other errors, like 403 etc, too.

@addyosmani
Copy link
Owner

We should be handling that a little more elegantly. Thanks for flagging! :)

I think some discussion probably needs to be had regarding what we do if a single file in a chain 404's like this: should an exception be thrown? should we continue loading the other items in the chain that may be dependencies? should the chain die altogether?

It would also be worth considering whether we should be handling anything for other HTTP errors that might occur.

//cc @sindresorhus @peol might have more thoughts on this :)

@peol
Copy link
Collaborator

peol commented Apr 17, 2012

Yeah, we have started some work in the api-rewrite branch on this, but we're only checking if it was OK (200), not sure how we're supposed to handle non-200 (404, 403, or whatever), maybe just try to continue executing the stack and if it breaks, it breaks?

@sindresorhus
Copy link
Contributor

We should IMO throw an exception, and fail the chain. It would most probably fail anyway. This way it's not a subtle fail.

@addyosmani
Copy link
Owner

@sindresorhus I think we should do that too. For some additional reference, what libraries like yepnope have looked at before are error timeouts (e.g if the script can't be loaded within a time window N, timeout and throw an exception). Their thread for reference SlexAxton/yepnope.js#61

@addyosmani
Copy link
Owner

If we implement let's go ahead and fail the chain if a 404 is encountered.

@sindresorhus
Copy link
Contributor

@felipemorais would you be interested in doing this one?

@felipemorais
Copy link
Contributor

@sindresorhus I'd like but I'm not sure what could be done.

Promise will be rejected if xhr.status !== 200.

Using:

basket.require({ url: 'jquery.js' },{ url: 'doesntexist123.js' },{ url: 'plugin.js' });

the second file is 404 but the first and third will be loaded because require is unordered.

@sindresorhus
Copy link
Contributor

Hmm, @wibblymat any thoughts?

@wibblymat
Copy link
Collaborator

The problem of 404s being saved into the cache, and of not being able to detect a failure, are both solved by the promises implementation.

It also gives the user more control over what happens when things go wrong. You could do:

// We absolutely definitely need jquery in order to display properly
basket.require({ url: 'jquery.js' })
    .then(function() {
        // But lets say that without the bootstrap js we can degrade gracefully if we know it
        basket.require({ url: 'bootstrap.js' })
            .then(siteWithBootstrap, siteWithoutBootstrap);
    });

However, looking at the current logic I'm not quite happy that I got it right.

Currently, if you do

basket.require({
    { url: 'jquery.js' },
    { url: 'doesntexist123.js' },
    { url: 'plugin.js' },
    { url: 'alsomissing.js' }
}).then(successHandler, errorHandler);

then jquery.js and plugin.js will be executed, but only the errorHandler will be called, and only once (for whichever failed first).

At the moment you can also do

basket.require({ url: 'doesntexist123.js' });
basket.require({ url: 'jquery.js' })
   .then(doStuff);

Due to the way it is written the promise returned from the second require encompasses both files, so doStuff wouldn't be called even if jquery loads - this is probably a bug. Seperate require calls shouldn't rely on each other.

I guess what we really want is:

  • The errorhandler should be called with details of all failures
  • The detail passed back should be specced out. At the moment it is an Error object with the message just being xhr.statusText
  • Scripts shouldn't be executed unless everything in a particular require() call succeeds. If you require 4 files at the moment, and get an error, you can't really tell what has run and what hasn't.

@felipemorais
Copy link
Contributor

@wibblymat
If require catch 404 once, no other will be executed?
I think that could be executed and pass what are success / failed on error handling.

@addyosmani
Copy link
Owner

I think the behaviour @wibblymat described as ideal is pretty much what we're after. Let's try to tackle this for 0.4 if we can agree on the implementation.

I appreciate the reminder about promises better handling this use case as I hadn't thought about them being if great assistance with this problem until now.

@peol
Copy link
Collaborator

peol commented Jan 1, 2013

I think it's fine that we're executing them as we pull them in. If the user is chaining stuff and something fails, he has an issue with his project and what basket.js in that case is rather minor. The fail handler does get called anyway so I'm fairly sure his project is in a dodgy state anyway since not all the required files could get pulled in.

If we're queueing up execution we could have a huge bottleneck instead, and I'm not sure we're gaining much from it.

@apaatsio
Copy link
Author

Closing as stale.

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

No branches or pull requests

6 participants