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

Revert "Add toStringTag value on JSG resource types" #1700

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

mikea
Copy link
Contributor

@mikea mikea commented Feb 20, 2024

Reverts #1607

Breaks downstream trace test

@mikea mikea requested review from a team as code owners February 20, 2024 19:16
@jasnell
Copy link
Member

jasnell commented Feb 20, 2024

@mikea ... how was the internal repo test broken? Does the test assume a particular output when an object to stringified? e.g. [object Object] vs. [object Foo]? Depending on exactly how that test was broken this change might end up having to go behind a compat flag.

@mikea
Copy link
Contributor Author

mikea commented Feb 20, 2024

@mikea ... how was the internal repo test broken? Does the test assume a particular output when an object to stringified? e.g. [object Object] vs. [object Foo]? Depending on exactly how that test was broken this change might end up having to go behind a compat flag.

hard for me to say what's wrong: there's so much output in the test. All I know is that Can log JSON edge cases subtest fails with this commit and passes without. @garrettgu10 has the build link (PR 7264)

@mikea mikea merged commit 83aafe5 into main Feb 20, 2024
11 checks passed
@mikea mikea deleted the revert-1607-ggu/to-string-builtin-objects branch February 20, 2024 19:48
@garrettgu10
Copy link
Contributor

@jasnell It looks like the test does assume a particular output when an object is stringified:

Expected: ["[object Object]"], actual: ["[object Headers]"]

@kentonv
Copy link
Member

kentonv commented Feb 21, 2024

It sounds like the test just needs to be updated, but it's important to merge those downstream updates at the same time as the workerd changes.

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.

4 participants