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: add a do-nothing SharedError.prepareStackTrace #1290

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

erights
Copy link
Contributor

@erights erights commented Sep 17, 2022

This PR does what I thought I understood to solve your depd problem. Does it?

If so, perhaps you should rebase #1286 on this and adapt your test so it passes. At the moment, this PR does not include any test for the added property, so your test would be most welcome

If not, please help me correct my misunderstanding.

Copy link
Contributor Author

@erights erights left a comment

Choose a reason for hiding this comment

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

(I'd request this change of myself if github allowed it)

One thing that's too weird about this PR as is, is that the do-nothing SharedError.prepareStackTrace would appear on all platforms, but InitialError.prepareStackTrace would not exist on non-v8 platforms. This needs to be more consistent, whether additively or subtractively. Waiting to understand the depd problem better before deciding.

@naugtur
Copy link
Member

naugtur commented Sep 19, 2022

I've pushed some tests, but they're failing on Error.captureStackTrace is not a function now. Would need to address that and I don't yet understand why it's not there.

@erights
Copy link
Contributor Author

erights commented Sep 20, 2022

I've pushed some tests, but they're failing on Error.captureStackTrace is not a function now. Would need to address that and I don't yet understand why it's not there.

It was not there because I had not understood that you needed it. It's there now.

See the changes I made to your test cases. Your first two passed, but I made them more precise. Your third one is impossible with a powerless Error object. But by creating a compartment endowed with the start compartment's powerful Error object, you should be able to do anything you can do with it in the start compartment.

In the endowed test case, captureStackTrace does work, but I could not make the substitute prepareStackTrace do anything. This is odd because the immediately following test case does, in order to support some depd functionality! There's something about it that I'm missing.

PTAL

@erights
Copy link
Contributor Author

erights commented Sep 20, 2022

The two new emulated properties:

  • SharedError.prepareStackTrace accessor for silently ignoring assignment
  • SharedError.captureStackTrace for assigning an empty string to its first argument.stack

are now added to SharedError only on v8. If we encounter code assuming these on other platforms, we can revisit.

The SharedError.stackTraceLimit accessor for silently ignoring assignment remains available everywhere. Due to Google's bad advice, this one does need to be available on all platforms.

@naugtur
Copy link
Member

naugtur commented Sep 21, 2022

In practice the assumption prepareStackTrace can only be available for writing in v8 may not be enough for Endo in particular, because the only way to invoke the version of depd that doesn't attempt setting it is using a bundler that prioritizes package.json's "browser" field to look for the module main. That's where depd exposes a compatible API that does nothing.

If we run codebase with depd in Endo we'll get into trouble again. For depd in particular a simple patch-package change to the package.json to define an export for Endo would do. Not sure about other packages.

Anyway, this is now a great improvement to ecosystem compatibility.

Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

The approach seems sane

@erights erights merged commit 705aef2 into master Sep 21, 2022
@erights erights deleted the markm-silently-inert-prepareStackTrace branch September 21, 2022 16:23
@erights erights mentioned this pull request Nov 7, 2022
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.

3 participants