Avoid wrapping the mod.fn and Y.use callback method with try/catch to get reasonable stacktrace. #69

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@opichals
Contributor

This is a pull request for ticket #2531679 change.

This is a repro case http://jsfiddle.net/aURCe/. The 'push on undefined' errors should be reported with a stacktrace that allows for easy problematic file/line location.

Owner

My issue with this change is that the errorFn config option can also negate the throwFail config. This change would make that ineffective.

If the erronFn config returns true, then the throw never happens. In this case that would not work. Even if throwFail was true and an errorFn was specified then it would not get triggered since you are not wrapping it any longer.

Contributor

Fair enough...

To keep the throwFail/errorFn semantics the same I see an option of introducing another config option (like 'failOnUseException' or something) which would be used in the code as shown below. What do you think?

                    if (mod.fn && Y.config.failOnUseException) {
                        mod.fn(Y, name);
                    } else if (mod.fn) {
                        try {
                            mod.fn(Y, name);
                        } catch (e) {
                            Y.error('Attach error: ' + name, e, name);
                            return false;
                        }
                    }
Owner

Let me see if I can make the exceptions show better first, I would rather not add another config option just for the use case.

Owner

From the simple test that I have, it seems that Firefox is the only one that handles "re-throwing" of errors properly:

https://img.skitch.com/20120118-eqkihh1mjcxt8csfqkndufd66u.jpg

According to this bug, seems people are complaining about Chrome already:
http://code.google.com/p/chromium/issues/detail?id=60240

Now we just need to determine what it is we want to do about it.

@rgrove, @lsmith, @ericf , @derek Thoughts?

Contributor
rgrove commented Jan 18, 2012

I don't know much about this particular part of the lib, but I do know that I'm not the only one who's banged his head on his desk many times as a result of YUI eating exceptions. Even if Chrome didn't have problems with re-thrown exceptions, I'd still say we should try to avoid re-throwing if at all possible.

In any case, in the proposed patch, there's an unnecessary re-evaluation of mod.fn if Y.config.throwFail or Y.config.failOnUseException (which is much too long a name, btw) is false. This really needs to be two if statements:

if (mod.fn) {
    if (Y.config.whatever) {
        // ...
    } else {
        // ...
    }
}
Contributor
lsmith commented Jan 18, 2012

Am I missing something or is this problem
A) purely a development time concern, and
B) obviated by throwFail: false in the YUI_Config in a development environment?

So if errorFn is defined, it doesn't matter how throwFail is set?

If that's the case, then wouldn't a test for

// Or to get all funky (Y.config.errorFn && /true/.test(Y.config.errorFn.toString())
// ignoring that function serialization is not a good practice[1]
var wrap = Y.config.errorFn || !Y.config.throwFail;

if (mod.fn) {
    if (wrap) {
        try { ... }
        catch (ex) { /* re-throw for what it's worth */ }
    } else {
        ...
    }
}

At issue is that errorFn reacts to an exception, and throwFail allows errorFn to work. If the browsers don't re-throw well, which it seems they don't, then we have a few imperfect options to support getting reasonable stack traces (in production environments?):

  1. Do nothing; devs can turn on throwFail (incomplete if they need an errorFn that never returns true)
  2. Lessen the impact with something like the snippet above,
  3. Add another config as suggested,
  4. Serialize the stack trace by hand (this is a PITA - see https://github.com/eriwen/javascript-stacktrace)
    N) I missed something

[1] I'd link to the article on perfectionkills.com, but it's blacked out in protest of SOPA. But kangax wrote a thorough article on the fragility of function serialization

Owner

@lsmith Setting throwFail to false will not help since the Exception is being "re-thrown" at a later point. That's the real issue here. The re-throwing of the error causes the stack-trace to change where the error actually occurred. In this example you can see that the line numbers are different and there is no way to get to the actual stack trace.

I have a branch that I'm about to commit that adds a alwaysThrow config option that will call the attach callback mod.fn and the use callback without the try/catch wrapper. This forces the Exception to be thrown in place and not modified at all.

This is definitely a dev time issue, and I have documented it as such.

Owner

Just pushed my solution to this issue:

davglass/yui3@155d51e

Contributor
rgrove commented Jan 18, 2012

I'm not really sure what value alwaysThrow adds. That's fine for experienced YUI devs and people who are willing to dig through the docs to find that option, but the vast majority of developers won't know about it and will still headdesk when YUI eats exceptions by default.

I feel like the right thing to do is to be noisy by default when an error occurs, because errors should not occur. If a developer doesn't feel like fixing their errors and wants to just hide them in production, then they can set a config value to hide them. But we shouldn't require devs to do extra work just to see meaningful errors in the first place.

Owner

I don't think I can remove this setup this late in the PR2 stage, that should have been in a PR1 since it very well could break a lot of things. I agree that the throwFail config is bogus since it's defaulted to true but it swallows the error and re-throws it.

If I am to change that default behavior then the errorFn that people rely on would not have any effect any more and would change the way we/they log messages.

Owner

I'm going to hold this until 3.6.0 PR1 since it will be a potentially breaking change, I'll document my changes on the ticket:

http://yuilibrary.com/projects/yui3/ticket/2531679

@davglass davglass was assigned Jan 30, 2012
Owner
davglass commented May 2, 2012

Fixed in b092a4b

@davglass davglass closed this May 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment