-
Notifications
You must be signed in to change notification settings - Fork 75
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
fix: tame Error constructor #359
Conversation
0aa101d
to
a04148a
Compare
I don't know if this is supposed to work or not, but this branch (at commit a04148a) did not allow a import 'ses';
import callsites from 'callsites';
lockdown();
console.log(callsites());
If I comment out the |
a04148a
to
2938dca
Compare
2938dca
to
d874a84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the base use case would work, so this would be a step forward for compatibility. The "safe" mode seems fine to me, and the "unsafe" mode can't get any more unsafe :). So I'd say go ahead and land this, but let's make sure we figure out the non-v8 and nesting cases before we call this done.
@warner to address the infinite recursion, enough changed that I re-requested a review. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Maybe add one comment, and take a brief look at my captureStackTrace
comment, but otherwise it looks ready for landing to me.
7e265f2
to
80bebd5
Compare
An initial taming of the Error constructor that should both be safe enough and compat enough for cases we've seen.
Build on the partial-intrinsic-reform branch, which set the stage.
Current a Draft PR because none of this is tested yet.