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(NODE-6348): Wrap thrown errors in JS Error objects with stacks #25

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

sophiebits
Copy link
Contributor

We noticed in our usage of this package that thrown errors (eg: "Unknown frame descriptor") don't have a useful .stack property. This makes debugging harder. It seems this is expected behavior for errors created via napi at least on V8; it seems that the best way to remedy this is to recreate Error objects on the JS side?

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

  • Ran npm run format:js && npm run format:rs script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@sophiebits
Copy link
Contributor Author

I don't have a cargo setup on my machine at the moment but crossing my fingers the test passes in CI. :)

@addaleax
Copy link

Hi @sophiebits! 👋 CI's pointing out a syntax error in the test, let us know if you'd prefer to take care of that yourself or we should be pushing a fix this branch 🙂

We noticed in our usage of this package that thrown errors (eg: "Unknown frame descriptor") don't have a useful .stack property. This makes debugging harder. It seems this is expected behavior for errors created via napi at least on V8; it seems that the best way to remedy this is to recreate Error objects on the JS side?
@sophiebits
Copy link
Contributor Author

Sloppy of me! Hope this will fix it. ^

test/index.test.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry about the messy tests)

index.js Outdated Show resolved Hide resolved
@sophiebits
Copy link
Contributor Author

  • updated to do the wrapping in a separate file
  • updated so the names at runtime are like just decompress not module.export.decompress
  • bindings have been regenerated; there are some minor changes since the last index.js update, perhaps because I'm on a newer napi? this is from napi 2.16.9. looks like mostly additional branches for possible architectures, not sure if this has a risk of breaking anything — lmk
  • (got my setup fixed and verified the tests pass locally for me now!)

@durran durran added the Team Review Needs review from team label Aug 28, 2024
@durran durran self-assigned this Aug 28, 2024
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution. I'll have the rest of the team look as well.

@durran
Copy link
Member

durran commented Aug 28, 2024

Updated the bindings.js to the old index.js per discussion on internal Slack with respect to some operating systems we don't support (like Android). I've pushed that change to this PR.

@durran durran merged commit af62c4f into mongodb-js:main Aug 29, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants