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

The revenge of long stack traces #311

Closed
wants to merge 2 commits into from
Closed

The revenge of long stack traces #311

wants to merge 2 commits into from

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Jun 4, 2013

This makes long stack traces infinite, and changes their API from longStackJumpLimit = 0 | 1 (default 1) to longStackSupport = true | false (default false). Notably, infinite long stack traces come with no real performance or memory penalty over our existing one-level "long" stack traces.

See #264 for some more discussion. In particular, should we switch on the presence of the Web Inspector console and/or (process.env.NODE_ENV || "development") !== "development" in order to turn them on by default?

This also adds a undocumented-for-now API promise.source to every promise that originates inside defer(), i.e. every promise except immediately-fulfilled and immediately-rejected ones. This could be fun for constructing promise graph visualizations, as I was discussing with @brendankenny at JSConf.

This replaces `longStackJumpLimit`, as that concept was not well-conceived. It also changes the default from turning on long stack traces to turning them off.

Fixes #264.
@domenic
Copy link
Collaborator Author

domenic commented Jun 6, 2013

@kriskowal after looking into it, Chrome removed the ability to detect if the console was open. And it was kind of hacky anyway, either cluttering your console with "Profile Started ... Profile Ended" or forcing Q to do a console.clear() and lose any console data before Q loads.

Combined with my slight unease about the dominant pattern for NODE_ENV defaulting to development, I think it's best to just leave the long stacks off by default. So, this pull request is good as is.

Any comments before I merge?

@domenic domenic mentioned this pull request Jun 6, 2013
@domenic
Copy link
Collaborator Author

domenic commented Jun 10, 2013

@kriskowal when you have time I think this is the only thing I want to get in before doing a 0.9.5. Then we can move on to more experiments like #313 and the nextTick stuff, mwuahaha.

@kriskowal
Copy link
Owner

This looks good. Would you like me to merge this and cut the release?

@domenic
Copy link
Collaborator Author

domenic commented Jun 10, 2013

Awesome :). Yeah, will do tomorrow—bedtime now!

On Jun 10, 2013, at 1:07, "Kris Kowal" <notifications@github.commailto:notifications@github.com> wrote:

This looks good. Would you like me to merge this and cut the release?


Reply to this email directly or view it on GitHubhttps://github.com//pull/311#issuecomment-19181017.

@domenic
Copy link
Collaborator Author

domenic commented Jun 10, 2013

Merged as 49d920d and 97f4c6b, w00t (and yay for avoiding merge commits!).

@domenic domenic closed this Jun 10, 2013
@kriskowal kriskowal deleted the long-stacks branch December 4, 2013 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants