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

fix(cli/js/error_stack): Expose Error.captureStackTrace #5254

Merged
merged 3 commits into from
May 29, 2020

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented May 12, 2020

@kevinkassimo
Copy link
Contributor

Not super sure if we should expose it as part of the type? Since it is technically a non-standard v8 only feature

@nayeemrmn
Copy link
Collaborator Author

Maybe... but nothing in https://v8.dev/docs/stack-trace-api suggests that this is for internal runtime use only -- in fact it makes lots of references to "user scripts". Unless Deno has a special reason to override this we should just let it pass through IMO.

Couple this with the fact that users do seem to be using this API, and that it will continue to exist in JS, not supporting it in TS just seems like a stumbling block.

@kitsonk
Copy link
Contributor

kitsonk commented May 13, 2020

I agree with @nayeemrmn. Deno is tightly coupled to V8, that means non-spec APIs supported by V8 are candidates for inclusion in Deno type definitions.

For reference, @types/node includes them: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/db4d8c51d2028805bfe722c83986f1693d27b9c2/types/node/v10/globals.d.ts#L131-L143.

@nayeemrmn nayeemrmn changed the title feat(cli/js/error_stack): Support Error.captureStackTrace feat(cli/js/error_stack): Expose Error.captureStackTrace May 13, 2020
@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) labels May 15, 2020
@bartlomieju bartlomieju modified the milestones: 1.1.0, 1.0.2 May 21, 2020
@nayeemrmn nayeemrmn force-pushed the capture-stack-trace branch from 0e34d7f to 5612cd8 Compare May 21, 2020 11:20
@nayeemrmn nayeemrmn changed the title feat(cli/js/error_stack): Expose Error.captureStackTrace fix(cli/js/error_stack): Expose Error.captureStackTrace May 21, 2020
@bartlomieju
Copy link
Member

@nayeemrmn please merge with master - I'll land this PR for 1.0.3

@bartlomieju bartlomieju removed this from the 1.0.2 milestone May 29, 2020
@bartlomieju bartlomieju merged commit 49c7077 into denoland:master May 29, 2020
@nayeemrmn nayeemrmn deleted the capture-stack-trace branch May 29, 2020 12:07
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Projects
None yet
4 participants