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

Improved Error types to work in browsers without v8 engine #40

Merged
merged 1 commit into from
Mar 19, 2015

Conversation

nodiis
Copy link
Contributor

@nodiis nodiis commented Mar 19, 2015

Error.captureStackTrace is only supported in browsers with v8 engine.
With this pull request I can use request-promise in all browsers.

@analog-nico
Copy link
Member

Awesome, thank you!

I wonder, if we should move Error.call(this) to the top to set name, message etc. afterwards. In V8 that is not necessary. But how about the other browsers? What would you prefer?

@nodiis
Copy link
Contributor Author

nodiis commented Mar 19, 2015

I tested this order in Firefox and IE (latest). So I would leave it as it is.

@analog-nico
Copy link
Member

I just looked into Error.call(this) and as far as i can tell now it doesn't work as intended. See the section "Obstacle 2: a constructor that can’t be called as a function" in http://www.2ality.com/2011/12/subtyping-builtins.html
Any knowledge you can dump on me?

@analog-nico
Copy link
Member

BTW, the test failed due to a timeout. Your PR was working fine.

analog-nico added a commit that referenced this pull request Mar 19, 2015
Improved Error types to work in browsers without v8 engine
@analog-nico analog-nico merged commit 1d11365 into request:master Mar 19, 2015
@analog-nico
Copy link
Member

One specific question: In Firefox and IE do the errors have a stack property? (According to the blog post they won't. But the blog post may be wrong.)

@nodiis
Copy link
Contributor Author

nodiis commented Mar 20, 2015

Firefox does have a non-standard stack property. See https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Error/stack

And IE also has a stack property. See
https://msdn.microsoft.com/library/ie/hh699850(v=vs.94).aspx/

So it would be possible to write the stack trace to the console.

Regarding the Error.call(this) the blog post seems right. Probably it would be the best to check for the stack property if Error.captureStackTrace is not available.

@analog-nico
Copy link
Member

Alright, I keep the if (Error.captureStackTrace) but remove the Error.call(this).

Thanks for your feedback!

analog-nico added a commit that referenced this pull request Mar 20, 2015
@analog-nico
Copy link
Member

I just published v0.4.1 on NPM.

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.

2 participants