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

Add Options.InteropOptions.BuildCallStackHandler #1793

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

scgm0
Copy link
Contributor

@scgm0 scgm0 commented Feb 25, 2024

A preliminary idea, this would allow one to modify the stack string generation, such as using a map of the source map.

@scgm0 scgm0 force-pushed the BuildCallStackHandler branch 2 times, most recently from 501229c to 9e95bea Compare February 25, 2024 20:08
@scgm0
Copy link
Contributor Author

scgm0 commented Feb 26, 2024

@lahma What do you think?

@lahma
Copy link
Collaborator

lahma commented Feb 26, 2024

Probably would need better documentation and tests, ValueStringBuilder shouldn't be made public. If there's a solid use case for such configuration, why not.

@scgm0
Copy link
Contributor Author

scgm0 commented Feb 26, 2024

Probably would need better documentation and tests, ValueStringBuilder shouldn't be made public. If there's a solid use case for such configuration, why not.

No longer exposing ValueStringBuilder to make BuildCallStackHandler return strings directly without manually calling ValueStringBuilder.
Thinking about how to write the tests, as for the documentation it might be later, because my English is very bad (been using translation software)

@scgm0
Copy link
Contributor Author

scgm0 commented Feb 26, 2024

@lahma This is my first time writing a test, is it enough?

@lofcz
Copy link
Contributor

lofcz commented Feb 27, 2024

If there's a solid use case for such configuration, why not.

We've discussed this over at Acornima adams85/acornima#6
This was previously requested here as well: #490

This should warrant a sufficient use case. As hinted in the included test the feature aims to enable running transpiled TypeScript (and possibly other JS dialects) through Jint with matching error lines etc. Note that debugging is already possible with remapping DebugInformation so this is more of a QoL improvement.

@lahma
Copy link
Collaborator

lahma commented Feb 27, 2024

This is starting to look nice with the example use case described with tests! I'm traveling until Saturday but probably with minor tweaks we can merge.

@lofcz
Copy link
Contributor

lofcz commented Feb 27, 2024

@scgm0 the PR is missing XML doc for the newly added public members, if you want I can add that for you.
Also thanks for implementing this, much appreciated.

@scgm0
Copy link
Contributor Author

scgm0 commented Feb 27, 2024

@scgm0 the PR is missing XML doc for the newly added public members, if you want I can add that for you. Also thanks for implementing this, much appreciated.

Thank you, I need it very much! As I said, my English is terrible, and inaccurate translation can be misleading. . .

@lahma
Copy link
Collaborator

lahma commented Feb 27, 2024

One thing that came to mind is that would it be enough just to have callback MapSourcePosition which would by default just return same values back or be null and call stack builder would just call it and all string manipulation would remain in original call stack builder.

@scgm0
Copy link
Contributor Author

scgm0 commented Feb 27, 2024

One thing that came to mind is that would it be enough just to have callback MapSourcePosition which would by default just return same values back or be null and call stack builder would just call it and all string manipulation would remain in original call stack builder.

The source map allows mapping function names(Although the typescript compiler does not seem to support it), and there may be users who prefer to use other formats for stack strings. For example, I like to use 4 spaces before at instead of 3.

@lahma
Copy link
Collaborator

lahma commented Feb 27, 2024

The current format is node format I guess. We also need to consider if CallStackBuilder could be inherited and methods overridden, maybe configurable properties for indent. Need to tinker a bit

@lofcz
Copy link
Contributor

lofcz commented Feb 27, 2024

@scgm0 I've opened a PR against your branch with the docs. Getting win/fw462 to pass the tests requires resigning the Tests assembly due to the added dependency on SourceMaps I believe, win/net8 is ok.

@scgm0
Copy link
Contributor Author

scgm0 commented Feb 27, 2024

@scgm0 I've opened a PR against your branch with the docs. Getting win/fw462 to pass tests requires resigning the Tests assembly due to the added dependency on SourceMaps I believe, win/net8 is ok.

Thank you so much!

@scgm0
Copy link
Contributor Author

scgm0 commented Feb 27, 2024

I'm sorry, I'm using linux and still don't know why the win test fails (

@lahma lahma force-pushed the BuildCallStackHandler branch 3 times, most recently from f6010ee to dafba58 Compare October 27, 2024 08:52
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Sorry it took so long, I rebased this and made minor adjustments, thank you for the contribution!

@lahma lahma enabled auto-merge (squash) October 27, 2024 08:58
@lahma lahma merged commit 7ac9805 into sebastienros:main Oct 27, 2024
3 checks passed
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