Currently when a behavior crashes, it stops everything.

The patch will catch any error happening during attach and detach loop and will throw the list of errors once all behaviors have been processed. Throwing at this point will not stop event handler to be executed, so things are all working as expected, except for the behavior that failed.

This will be make things much easier on contrib.

For testing yo can add FAIL++; or throw "my error"; in any behavior (try adding that to two behaviors to see what happens when you have several error on attach/detach).

Follow up issues

#1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors

Files: 
CommentFileSizeAuthor
#68 core-js-behavior-guard-throw-1639012-68.patch3.79 KBnod_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,574 pass(es).
[ View ]
#58 after_patch_58.gif158.28 KBcorbacho
#58 before_patch_58.gif317.07 KBcorbacho
#55 drupal8.js-errors.48.break-it.do-not-test.patch2.77 KBsun
#55 interdiff.txt337 bytessun
#53 logging-and-errors.png87.12 KBsun
#49 drupal8.js-errors.48.patch2.37 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,671 pass(es).
[ View ]
#37 js-1639012-37.patch2.74 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 41,986 pass(es).
[ View ]
#27 core-js-try_catch-behaviors-1639012-27.patch2.92 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 39,353 pass(es).
[ View ]
#17 core-js-try_catch-behaviors-1639012-17.patch2.75 KBseutje
PASSED: [[SimpleTest]]: [MySQL] 40,365 pass(es).
[ View ]
#16 Screen shot 2012-08-22 at 13.09.03.png91.46 KBJelle_S
#16 Screen shot 2012-08-22 at 13.09.35.png91.86 KBJelle_S
#14 core-js-try_catch-behaviors-1639012-14.patch2.73 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 39,742 pass(es).
[ View ]
#11 core-js-try_catch-behaviors-1639012-11.patch2.7 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 37,003 pass(es).
[ View ]
#6 core-js-try_catch-behaviors-1639012-6.patch2.24 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 37,027 pass(es).
[ View ]
core-js-try_catch-behaviors.patch1.78 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 36,893 pass(es).
[ View ]

Comments

nod_’s picture

Priority:Normal» Major
Issue tags:+needs backport to D7

tag

merlinofchaos’s picture

If this works (should be fairly easy to test) I'm totally for this.

nod_’s picture

For the sake of easy testing please copy/paste this code in any JS file already included in the install (at the bottom of drupal.js for example, that works) and open up node/add/article. With and Without the patch.

Drupal.behaviors.BreakEverything = {
  attach: function () {
    console.log("this is bad, I'm going to fail");
    FAIL++;
  }
};

Drupal.behaviors.meTooBreakEverything = {
  attach: function () {
    console.log('This is reaaaally bad');
    throw new Error("Oups");
  }
};

SebCorbin’s picture

Status:Needs review» Needs work

Works great in latest Chrome and Firefox (needs IE test), one thing though:

+++ b/core/misc/drupal.jsundefined
@@ -103,13 +113,23 @@ Drupal.detachBehaviors = function (context, settings, trigger) {
+  if (errors.length) {
+    throw errors.join(', ');
+  }

Shouldn't this be a loop on errors? Merging them into a single string make it difficult to process them later, e.g.

for(i in errors) {
  throw errors[i];
}
nod_’s picture

Status:Needs work» Needs review

Looping will only throw the first error and stop, so we either make our own error type to give access to the raw list, using this kind of things #1606362: Use throw when an ajax error occurs and extend JavaScript's Error object or we throw everything in one string like the patch currently does.

nod_’s picture

StatusFileSize
new2.24 KB
PASSED: [[SimpleTest]]: [MySQL] 37,027 pass(es).
[ View ]

All right, made a proper error object, now the raw list can be accessed, meaning this will work:

try {
  Drupal.attachBehaviors();
}
catch (errors) {
  errors.list; // the array of {behavior: 'name', error: 'object'} objects.
  errors.message; // serialized list of errors like in #0.
}

I guess it's the best of both world. Don't really know where the error definition should live and if it should be possible to alter (meaning we put it in Drupal.AttachError or something).

SebCorbin’s picture

All good for me, but it needs some other browser tests (dear IE)

nod_’s picture

Issue tags:+Needs manual testing

No surprises for this kind of things cross-browser, it should work fine. Still need to be tested though :)

seutje’s picture

This sort of error handling is a must!

will definitely test if I find a moment to do so.

nod_’s picture

Works fine on IE8 (not the emulated version).

nod_’s picture

StatusFileSize
new2.7 KB
PASSED: [[SimpleTest]]: [MySQL] 37,003 pass(es).
[ View ]

Better name, doc, say if it's during attach, detach + trigger.

Happy with it like that, it works. Might need to rename one or two variable and have better docs so please help out with that if you feel it's needed.

It's totally backportable too.

(edit) the only thing we need to agree on before rtbc is naming and docs, that's pretty much it.

RobLoach’s picture

Are there any places where we could make use of this? Maybe on the AJAX errors? I know up until recently, it just gave you a big ol' message box error.

[EDIT] Added the issue as a follow up in the issue summary.

sun’s picture

Status:Needs review» Needs work
+++ b/core/misc/drupal.js
@@ -10,6 +10,28 @@ jQuery.noConflict();
/**
+ * Custom Error thrown after attac/detach if one or more bebavior failed.
+ *
+ * @param list
+ *   An array of errors thrown during attach/detach
+ * @param event
+ *   attach or detach
+ */

1) Typo in "attac"

2) Typo in "bebavior" (should also be plural, I think)

3) Missing final sentence period in list param description.

4) Any chance to make the event param description less cryptic? :)

5) It would be great to clarify how this Error "behaves" for developers; e.g., is it output as an alert? In the console? Does the behavior depend on the browser? Does it depend on the error(s)? Lots of unknowns here. :)

+++ b/core/misc/drupal.js
@@ -10,6 +10,28 @@ jQuery.noConflict();
+  // Pretty print the list of errors.
+  var messageList = [];
+  messageList.push(this.event);
+  for (var i = 0, il = this.list.length; i < il; i++) {
+    messageList.push(this.list[i].behavior + ': ' + this.list[i].error.message);
+  }
+  this.message = messageList.join(' ; ');

1) It's not clear to me why we're building a second stack of messages and joining that in the end, instead of producing the pretty output within that loop directly?

2) Speaking of pretty, any chance to inject newlines between the messages (as opposed to that " ; ", which I guess doesn't really produce "pretty" output)? [Depends on the final output area/context, I guess...]

+++ b/core/misc/drupal.js
@@ -10,6 +10,28 @@ jQuery.noConflict();
+DrupalBehaviorError.prototype = new Error();

Is it proper to instantiate a new Error for the prototype (as opposed to just referencing it)?

+++ b/core/misc/drupal.js
@@ -103,13 +135,23 @@ Drupal.detachBehaviors = function (context, settings, trigger) {
+    throw new DrupalBehaviorError(errors, 'detach:' + trigger);

A dot/period between detach and trigger might make more sense?

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new2.73 KB
PASSED: [[SimpleTest]]: [MySQL] 39,742 pass(es).
[ View ]

Oh wow a review! Thanks so much sun :)

1, 3, 4, fixed.

2. We might need a grammar guru, a plural there sounds weird to me.

5. It's a regular JS error, You'd get the same thing as if you'd write FAIL++ anywhere in the JS (and without declaring the FAIL variable before of course). It's just the error type that would be different since it's a custom one here. So no unkonwn to me, it's a JS error and we just throw it like any other error.

6. Well, it's to prevent people from doing stupid things. In JS you should always separate the read and write part (for DOM manipulations). Now this is not a DOM manipulation but here are my assumptions: 1) people copy core code to do their thing (especially in JS who has a huge copy/pasing "culture"). 2) people will usually want to build an HTML string and use .innerHTML or $.html(). doing anything to .innerHTML within a loop is a huge mistake. It doesn't make a real difference and "train" people to do the right thing.

Also it's more practical, I don't have to test if whatever is the last item of the list and not add a ";". Changing the variable type from an array to a string to keep the practical aspect of things is just wrong, it's not because it's not strongly typed that we need to abuse it.

7. Yep, tried that. tried overriding the toString() method as well. Browsers are just doing whatever they want. newlines are munged in IE making the output unreadable. overriding the toString is inconsistent in Chrome/FF since one prefix whatever by the error type and the other just take the toString() output. I'm not doing browser sniffing to pretty print a failure :)
It's just an error, it needs to be readable, not pretty, changing the comment. If they need pretty they have access to the complete list of raw errors.

8. Yep that's the way you inherit things. And no new means the prototype is directly the Error constructor, that's no good. Talk to msonnabaum, he asked me the same question and checked how coffescript did it, coffescript does just that :)

9. That would mislead people to think it's a namespaced event, it's not. Happy to change that to anything but a dot.

Thanks again for the review, happy to explain every and anything JS :)

sxnc’s picture

Tested this on my local and it did started throwing me an error caused by the admin overlay wich wasn't caused by the patch but by a badly installed Drupal :p

But it was a perfect test, so the patch worked fine :)

(also did some other js errors including the example from comment #3)

Jelle_S’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new91.86 KB
new91.46 KB

I wrote a quick Testswarm test for this patch:

/**
* Tests Drupal.attachBehaviors() with a failing behavior. See #1639012
*/
Drupal.tests.drupalBehaviorsGuard = {
  getInfo: function() {
    return {
      name: 'Drupal.attachBehaviors() Guard',
      description: 'Tests for Drupal.attachBehaviors() with behaviors that throw errors.',
      group: 'System'
    };
  },
  setup: function() {
    this.originalBehaviors = Drupal.behaviors;
    Drupal.behaviors = {
      undefinedVar: {
        attach: function () {
          console.log("this is bad, I'm going to fail");
          FAIL++;
        }
      },
      throwError: {
        attach: function () {
          console.log('This is reaaaally bad');
          throw new Error("Oups");
        }
      },
      validBehavior: {
        attach: function () {
          // Just put an ok statement here. If previous behaviors fail, and there is no guard
          // this behavior will not run, and Qunit will complain: "Expected 1 assertions, but 0 were run".
          ok(true, Drupal.t('Behavior ran after failing behaviors.'));
        }
      }
    };
  },
  teardown: function() {
    Drupal.behaviors = this.originalBehaviors;
  },
  tests: {
    behavioursGuard: function ($, Drupal, window, document, undefined) {
      return function() {
        expect(1);
        // Test attaching failing behaviors.
        try {
          Drupal.attachBehaviors('Attach context 1', 'Attach settings 1');
        }
        catch(e) {
          // Catch the errors after all behaviors have executed, or the test will fail anyway.
        }
      }
    }
  }
};

(see 855a099)

Before patch:
Screen shot 2012-08-22 at 13.09.03.png

After patch:
Screen shot 2012-08-22 at 13.09.35.png

seutje’s picture

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 40,365 pass(es).
[ View ]
+++ b/core/misc/drupal.jsundefined
@@ -10,6 +10,28 @@ jQuery.noConflict();
+ * Custom error type thrown after attach/detach if one or more bebavior failed.

Fixed typo.
Not a grammar guru, but singular sounds wrong, changed it to a plural.

+++ b/core/misc/drupal.jsundefined
@@ -10,6 +10,28 @@ jQuery.noConflict();
+ *   The 'attach' or 'detach' string.

Sounds kinda weird, changed it to "A string containing either 'attach' or 'detach'."

Didn't test functionality as the QUnit test in #16 covered that rather well.

nod_’s picture

Status:Needs work» Needs review

Works for me :)

seutje’s picture

So I guess we just need sun to review his objections?

nod_’s picture

I asked him about in IRL on Saturday. He told me we shouldn't hang the issue on his feedback.

If you're happy with it, go for it :)

Jelle_S’s picture

Status:Needs review» Reviewed & tested by the community

Patch looks sound to me, only comments have been changed since #14, so tests from #16 still apply.

seutje’s picture

Has my blessing.

seutje’s picture

Damn wireless double post.

Wim Leers’s picture

Also looks good to me :)

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

klonos’s picture

...according to #11 this can be backported to D7. Right?

nod_’s picture

Version:8.x-dev» 7.x-dev
Category:task» feature
Priority:Major» Normal
Status:Fixed» Needs review
StatusFileSize
new2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 39,353 pass(es).
[ View ]

Here is a patch but it needs to be very carefully tested. It changes a test for behaviors. I'm pretty sure it'll be fine but we may end up un-breaking stuff that people rely on.

sun’s picture

Lovely, thanks everyone :)

I'd suggest to keep D7 alone... this change would introduce a "sudden" difference (sorta API change) with regard to how errors are handled in Drupal behaviors, which might produce some unexpected havoc. Although that's just my feeling, feel free to disagree. ;)

nod_’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Closed (fixed)

agreed.

sun’s picture

Status:Closed (fixed)» Active

Given #1776030: Machine name JavaScript behavior is broken, since jquery.once isn't loaded, in hindsight, I'm not sure whether this was a good idea... ;)

For almost one entire week, no one seems to have noticed that the machine name behavior is completely broken in HEAD... Only by knowing that "something" is missing on a page and opening the JS console, (potentially tons of) errors are revealed.

Don't get me wrong, I really liked this idea, because I regularly have to deal with lots of bogus JS... but now that I experienced the consequences of this change, it really has a huge touch of "babysitting broken code".

So I wonder whether this is actually a good idea, or whether we shouldn't revert it... :-/

What do you think?

Jelle_S’s picture

I don't think this should be reverted. Currently there's a lot of work going on in Drupal JS, but once the core JS is stable again, this is an absolutely great patch. We have some javascript-heavy modules (Clientside Validation in particular) and we get a lot of people saying "it doesn't work", when after a lot of debugging, the cause lies with an other js file throwing errors. Right now we just need to test a bit more before committing a js patch, but once again, once js is stable again, I think this is an excellent patch.

nod_’s picture

Last time I talked with merlinofchaos he was for the change, might want to see what's his opinion.

Wim Leers’s picture

I do see your point sun, but I have to agree with Jelle's reasoning.

Also note that this wouldn't be a problem at all if we had tests for JS.

nod_’s picture

The thing is, when it breaks and doesn't execute the rest of the behaviors, it actually show up the same in the console, it's just that your page is half-broken. We don't babysit bad code but we do babysit UX.

aspilicious’s picture

But at least that gives you the power to shoot down js developers and force them to fix it :p

nod_’s picture

What's the status on this? I want to close it to clean the JS feature request queue.

tim.plunkett’s picture

Status:Active» Needs review
Issue tags:-Needs manual testing, -needs backport to D7
StatusFileSize
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 41,986 pass(es).
[ View ]

I'd really like to see this reverted.
To properly debug an error I had, I had to revert this patch just to be able to find it.

This removes the try/catch while retaining the improvement of the for loop.

nod_’s picture

All the relevant informations and original errors that are catched from attachBehaviors like so:

try {
  Drupal.attachBehaviors();
}
catch (e) {
  console.log(e.list);
}

Also note that decent browser will provide tools to look into everything in the console/debug and access the original error thrown from inside the crashing behavior.

I'm not warming up to the idea of reverting since everything needed to debug is available with proper use of browser tools and developing a website/module is a tiny fraction of the time compared to having a website running where this is handy as far as users are concerned.

nod_’s picture

Status:Needs review» Fixed

8 weeks, toolbar patch got in, edit module has a ton of JS and no other complaints? back to fixed.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

catch’s picture

Status:Closed (fixed)» Active

Re-opening again after #1879240: Cannot add new filter group.

tim.plunkett’s picture

Status:Active» Needs review

In the views issue, in HEAD, here is the message in the console:

Uncaught DrupalBehaviorError: attach ; viewsUiRearrangeFilter: i is not defined drupal.js:93
Drupal.attachBehaviors drupal.js:93
Drupal.ajax.commands.viewsSetForm ajax.js:16
Drupal.ajax.success ajax.js:440
ajax.options.success ajax.js:202
fire jquery.js:974
self.fireWith jquery.js:1082
done jquery.js:7788
callback jquery.js:8500

With the patch in #37:

Uncaught ReferenceError: i is not defined views-admin.js:569
Drupal.viewsUi.rearrangeFilterHandler.insertAddRemoveFilterGroupLinks views-admin.js:569
Drupal.viewsUi.rearrangeFilterHandler views-admin.js:511
Drupal.behaviors.viewsUiRearrangeFilter.attach views-admin.js:484
Drupal.attachBehaviors drupal.js:60
Drupal.ajax.commands.viewsSetForm ajax.js:16
Drupal.ajax.success ajax.js:440
ajax.options.success ajax.js:202
fire jquery.js:974
self.fireWith jquery.js:1082
done jquery.js:7788
callback jquery.js:8500

The actual problem was in views-admin.js, around line 569. Which is what the second error message told me. The first, from HEAD, told me where the exception was caught, which was useless.

nod_’s picture

Firefox has useful debug informations.

tim.plunkett’s picture

I'm on Chrome 24, the latest version. Saying that FF works doesn't really help me. I'm pretty sure Chrome has a bigger marketshare at this point as well.

nod_’s picture

Filled #1891496: Remove JS behaviors guarding to help debugging for the devel module since that's when not guarding is useful.

tim.plunkett’s picture

Status:Needs review» Fixed

I RTBC'd the devel issue, this is a great compromise. Thanks nod_!

sun’s picture

Status:Fixed» Active

Why doesn't the JS error reporting correspond to the PHP error reporting?

We have a dedicated configuration UI to manage error reporting. We should re-use it.

nod_’s picture

Status:Active» Fixed

Tried, we need more choices in the UI and actually #1419652: [Meta] javascript logging and error reporting

Here is where I tried to do what sun suggests #1419648: javascript error reporting interface it just adds too much code that we don't really need. This issue is fixed, the other one needs reopening or a follow-up added to the meta if that's what we want.

sun’s picture

Status:Fixed» Needs review
StatusFileSize
new2.37 KB
PASSED: [[SimpleTest]]: [MySQL] 50,671 pass(es).
[ View ]

There.

nod_’s picture

and the configuration doesn't mean much with that.

if you hide errors, you'll run into the throw and the errors will be displayed defeating the purpose of not showing errors.

sun’s picture

You're referring to:

Hiding errors != logging errors

What you're missing is that an error reporting config that tells the app to REPORT errors actually reports errors and doesn't hide them away in log messages.

Logging errors != errors breaking the app.

Just logging was the original intention of this issue. But unconditionally hiding away errors causes actual errors to go unnoticed.

Whether still logging errors when errors should be hidden is legit, is a slightly different question. Once you introduce logging, new responsibilities for you. ;)

What matters for developers is that error reporting works in the way they expect it to work. Mirroring the PHP error reporting settings into JavaScript seems to be the most sensible compromise we can find for this issue, in order to make all sides happy.

The patch in #49 makes sure to not output an additional JS setting when error reporting is hidden; i.e., bandwidth is saved in production.

nod_’s picture

No, read the issue please I'm not talking about error logging. The meta is merely there to show the place where it should be discussed further.

Let's assume you have a behaviors that breaks.

With your patch and error reporting on, it all break and I see the error.
Now if I turn error reporting off, the behavior crashes, it's catched and it's thrown at the end. I see the error.

Now where does that leaves me knowing I explicitly checked: Errors message to display: None.

sun’s picture

StatusFileSize
new87.12 KB

Now where does that leaves me knowing I explicitly checked: Errors message to display: None.

Um. There: (emphasis mine)

logging-and-errors.png

nod_’s picture

Whatever I check, If a behavior is broken, an error will be shown in the console. Try it.

(and we've been off topic for a while too).

sun’s picture

StatusFileSize
new337 bytes
new2.77 KB

Works exactly like it should for me. Steps to reproduce:

  1. Apply attached patch (same as before, just with additional error in text.js).
  2. Visit /node/add
  3. Hard-reload the page to refresh the browser JS cache.
  4. Depending on your error reporting configuration on /admin/config/development/logging (which you can open in a separate tab):
    • if set to hide errors, all behaviors except of text.js are working in the node form.
      (e.g., vertical tabs appear correctly, errors are hidden away and silently ignored)
    • if set to anything else, all JS behaviors that would be executed after text.js are broken in the node form.
nod_’s picture

if set to hide errors, all behaviors except of text.js are working in the node form.
(e.g., vertical tabs appear correctly, errors are hidden away and silently ignored)

And this will show up as an error in the JS console: DrupalBehaviorError: attach ; textSummary: error is not defined.

nod_’s picture

Status:Needs review» Closed (fixed)
nod_’s picture

Issue summary:View changes

Updated issue summary.

corbacho’s picture

Issue summary:View changes
Status:Closed (fixed)» Needs review
StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,243 pass(es).
[ View ]
new317.07 KB
new158.28 KB

Sorry to reopen this, but debugging Drupal behaviors has become a *** pain.

Although it seems a good idea to catch-rethrow, (I agree that adds robustness to our JavaScript execution). It's a pain.
I'm sure many developers will get confused, when they see errors in their console, pointing to drupal.js (it's there where the exception is re-thrown, but not where the error is)

I tested in Chrome DevTools, FireFox DevTools,Safari, IE11.. all of them points to "drupal.js" as the culprit. The *only* tool that shows correctly the source of the error was Firebug.

Fail-fast approach has its advantages too. We have done it like that before in Drupal 7. And Even in JQuery they don't try-catch the $.deferreds or not even the $(document).ready stack of functions. If something fails, better to know as soon as possible.

But then I saw how React library is approaching a similar problem:
https://github.com/facebook/react/blob/master/src/utils/Transaction.js#L...

It uses try-finally to let bubble up the first error, and then executes the rest of the callbacks silently in the background.
It's quite clever way of handling it, and probably a good compromise.

Patch included following this approach. And gifs before and after the patch.
Tested in IE9 too.
EDIT: The patch needs a bit more work. behaviors_done should shouldn't be outside of the scope. It's problematic when a behavior calls attachBehaviors.

sun’s picture

Category:Feature request» Task
Issue tags:+DX (Developer Experience)

Thanks, @corbacho. I wasn't aware that this issue was prematurely closed (again).

That approach looks indeed very interesting; seemingly resolves the original goal in a better way (on the assumption that it actually works).

nod_’s picture

It got closed because you sort of derailed the issue in #47 sun.

+1 for 58, really great find. Indentation needs work and we might want to switch to the Object.keys().forEach() form to loop over the behaviors and remove some variables. But I'm all for it if it makes people stop complaining.

corbacho’s picture

StatusFileSize
new3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,335 pass(es).
[ View ]

Thanks.
I attached a working patch. I needed to add a third argument to Drupal.attachBehaviors to make the recursion works, but I found here and there strange side-effects, specially when Drupal.attachBehaviors is called inside of a behavior, that behavior won't continue executing. And, even is a cool trick code is getting complex and hairy... recursion makes everything more difficult than it should be.

Luckily I found an alternative approach that I will put in comment #62
Anyway, working patch attached for future reference.

corbacho’s picture

StatusFileSize
new2.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,301 pass(es).
[ View ]

Calling "throw" asynchronously with setTimeout, it won't halt the behaviors execution.
Patch attached.

Much more simple code, the console shows correct error information and all behaviors are executed. I like this approach. Re-throwing exceptions (instead of silenc'ing them) also makes possible to use tools like TraceKit, that captures the exceptions at a global level.

The only thing that doesn't work very well is the blue icon-button in Chrome Debugger for "Pause on exceptions" , (it will pause on the throw). The react way (patch #61) makes it work, but it's overkill solution, too complex. I don't think the "Pause on exceptions" feature is that important, in the meanwhile the console info and links there are correct.

@nod_ I'm using Object.keys. It's supported in modern browsers and we can save the "hasOwnProperty" check too. Good idea, but more work for the IE8 module

nod_’s picture

StatusFileSize
new3.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,432 pass(es).
[ View ]

Looked a bit more into it, patch up there looks good.

I'd keep the error list around and throw things only at the end after everything been processed.

As long as we're dealing with strict mode violation, or custom Errors (using throw new Error('');) Things will work as expected. If there is a throw "some string"; things won't work as expected. The line and file referenced will be useless.

corbacho’s picture

I like #63 patch. I tested and works ok. It's a good idea to name the (previously anonymous) function "runBehavior". This gives more info in the callstack.

I don't know why we need to collect the errors, we could throw them on the fly, setTimeout is taking care already of "grouping" the information after attachBehaviors is executed (since the attachBehaviors loop is synchronous). And it will make code a bit more readable.

Do you think is a good idea to make public the encapsulated function "throwError" ? For example, defined in Drupal.throwError, so it can be reused by detachBehaviors ? And the second benefit is that it could be overridden if someone needs to capture the errors for example. Or maybe devel module could re-define it for some purpose.

nod_’s picture

Since we're already throwing the timeline away by using setTimeout we might as well separate cleanly the steps for collecting faillures and throwing them. I'm not a fan of throwing from the catch.

Probably a good idea to expose the list of faillures externally, I don't have a good idea for what it'd look like though.

corbacho’s picture

StatusFileSize
new3.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,689 pass(es).
[ View ]

true, It's cleaner to read than setTimeout.
This is patch #63 with a Drupal.throwError helper to DRY'it up.
I don't know how useful is to expose the actual errors array. If someone needs them programmatically, they can use the window.onerror handler to capture errors

corbacho’s picture

Issue tags:+drupalcampfi
nod_’s picture

StatusFileSize
new3.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,574 pass(es).
[ View ]

This is still interesting to get in, it removes some code from drupal.js too and that's never bad.

Doesn't appear to be working in chromium anymore. FF is fine.

nod_’s picture

Status:Needs review» Postponed (maintainer needs more info)

Do people still need it?
What can we do now that it doesn't work on chrome anymore?