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

[WIP] catch jsonp error #692

Closed
wants to merge 6 commits into from
Closed

Conversation

zxcabs
Copy link

@zxcabs zxcabs commented Jan 15, 2015

Here pull request for #686

@sokra
Copy link
Member

sokra commented Jan 15, 2015

  • Can you put this code in an optional Plugin (JsonpErrorHandlingPlugin or similar)? I don't want to blow up the webpack runtime when you don't want error handling.
  • You must not change the require.ensure interface. It's expected that the callback is only called when the chunk is loaded. But require.ensure has no error handling specified :(. Maybe we should return a Promise from require.ensure? (I'm open for ideas)
  • use var a=1;var b=2; instead of var a=1,b=2;

@zxcabs
Copy link
Author

zxcabs commented Jan 15, 2015

  • ok
  • I think that require.ensure callback should always called and first arguments must be a Error or null, because it is a common behavior in callback style. But it breaks backward compatibility. As for the called only when the chunk is loaded, this code may simulate such behavior.
var
    defaultEnsure = __webpack_require__.e;

if (defaultEnsure) {
    __webpack_require__.e = function (chunk, callback, name) {
        defaultEnsure.call(null, chunk, function (error, _require) {
            if (!error) callback(_require);
        }, name);
    }
}
  • ok

@zxcabs zxcabs changed the title catch jsonp error WIP catch jsonp error Jan 15, 2015
@zxcabs zxcabs changed the title WIP catch jsonp error [WIP] catch jsonp error Jan 15, 2015
@zxcabs
Copy link
Author

zxcabs commented Jan 19, 2015

@sokra ?

@sokra
Copy link
Member

sokra commented Jan 20, 2015

I can take a look this evening...

/******/ script.onload = script.onreadystatechange = function() {
/******/ var readyState = this.readyState;
/******/ if (!readyState || readyState === "loaded" || readyState === "complete") {
/******/ end();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the requirejs docs they reckon older IE's will call the onreadystatechange handler with this.readyState set to 'complete' even if the script 404'd. I haven't got access to IE to try this out at the moment but I think it might be worth testing this and as a potential fix check for installedChunks[chunkId] === 0 as you can be sure the script has downloaded successfully if the jsonp callback has been run.

@richardscarrott
Copy link

This plugin looks really good, something I'm in need of. I wonder however if AMD support could be added without too much difficulty as, imo, AMD lends itself to error handling better than commonJS using this:

#758

require(['a'], function(a) {
    // success
}, function() {
    // error
});

I also wonder if it would be better to follow a similar signature for commonJS as that way it wouldn't be a breaking change:

require.ensure(['a'], function(require) {
    var a = require('a');    
    // success
}, function() {
    // error
});

The only slight complication would be the need to check the type of the third argument to determine if it's the 'name' or an error callback.

/******/ if (error) {
/******/ installedChunks[chunkId] = -1;
/******/ chunkLoadErrors[chunkId] = error;
/******/ var callbacks = installedChunks[chunkId];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work as you set installedChunks[chunkid] to -1 then try to retrieve the callbacks right after?

@zxcabs
Copy link
Author

zxcabs commented Feb 4, 2015

At work, I use a similar plug-in, but with a few changes. I saved the default order on arguments in require.ensure callback function for backward capability with webpack loaders and notify about timeout via window event. Tonight or tomorrow morning I can show it.

As for the interface require.ensure, I have come to believe that the best way is to return the Promise, but i don't know what to do for old browsers.

@richardscarrott
Copy link

I personally am not so keen for promises in this situation as it's a massive departure from "standard" commonJS and AMD specifications.

I think require.ensure(name?, deps, successCallback, errorCallback) would be the best as it's analogous to AMD (require(name?, deps, successCallback, errorCallback)) so would keep things simple and familiar.

I've noticed require.async uses this format too.

It'd be good to see what you're using currently.

@sokra
Copy link
Member

sokra commented Feb 4, 2015

I think require.ensure(name?, deps, successCallback, errorCallback) would be the best as it's analogous to AMD

looks great... and this way we could keep backward compatiblity...

@richardscarrott
Copy link

I've sent a pull request which adds support for the error callback when using require.ensure

#763

@jhnns
Copy link
Member

jhnns commented Mar 23, 2015

Any progress on this? I could use that too...

@richardscarrott
Copy link

I wrote a plugin https://github.com/richardscarrott/require-error-handler-webpack-plugin which adds error handling to require.ensure, AMD and the bundle loader, it was relatively difficult to hook into all the right places so would be nice if we'd be able to either properly integrate this into webpack or open up a few more hooks to make this plugin cleaner.

@indianburger
Copy link

Would be really useful if this merged

@NekR
Copy link

NekR commented Sep 3, 2015

@indianburger as a temporal solution you could use this: https://github.com/NekR/async-module-loader
Works pretty well and in most cases you just need to replace bundle-loader with async-module-loader.

@sokra sokra closed this in bf1f114 Nov 6, 2015
@jharris4
Copy link
Contributor

I've sent a pull request which adds support to Webpack v2.2 for the error callback when using require.ensure

#4069

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

Successfully merging this pull request may close these issues.

None yet

7 participants