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

14.6+ changed C++ addon behaviour of toString #35365

Closed
nschonni opened this issue Sep 26, 2020 · 7 comments
Closed

14.6+ changed C++ addon behaviour of toString #35365

nschonni opened this issue Sep 26, 2020 · 7 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. wontfix Issues that will not be fixed.

Comments

@nschonni
Copy link
Member

  • Version: 14.6
  • Platform: Windows, Linux, OSX, Alpine
  • Subsystem: x86 and x64

What steps will reproduce the bug?

Calling .toString on an object now returns [object Object], instead of [object TheType]

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

The type of the object should be printed like [object TheType]

What do you see instead?

Calling toString returns [object Object]

Additional information

You can see test covering this started to fail in 14.6 and above sass/node-sass@1f6b7f8, but are passing for previous node majors, and ok with 14.5 sass/node-sass@fd7ac32

Example test that fails on 14.6+

https://github.com/sass/node-sass/blob/1bab0ad3c888f9de8099d7faa750ba4fc83e9c0a/test/types.js#L17-L23

Node-sass used to be covered by CITGM, but maybe it was removed

/cc @xzyfer

@nschonni
Copy link
Member Author

Ah, looks like it was disabled in CITGM in nodejs/citgm#790, before 14.6 came out

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 27, 2020

v8/v8@1aa51b498e2 is the change and chromium:793406 the upstream bug report (tl;dr spec conformity.)

I'll close this out: not a bug and not under our control.

edit: forgot to mention: the V8 8.3 -> 8.4 upgrade in v14.6.0 contains that change.

@bnoordhuis bnoordhuis added v14.x v8 engine Issues and PRs related to the V8 dependency. wontfix Issues that will not be fixed. labels Sep 27, 2020
@nschonni
Copy link
Member Author

Thanks @bnoordhuis I figured it was probably from the v8 change. Is there any hint to implementing the @@toStringTag in a why that maintains backwards compatibility with 10 and 12? It used to be picking up the value from https://github.com/sass/node-sass/blob/1bab0ad3c888f9de8099d7faa750ba4fc83e9c0a/src/sass_types/color.h#L18

@bnoordhuis
Copy link
Member

Let's say C is your class, then C.prototype[Symbol.toStringTag] = C.name makes .toString() return '[object C]', and that works with v10.x and newer.

@nschonni
Copy link
Member Author

Thanks, I'm trying to figure it out one the C/C++ side rather than the JS side

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 27, 2020

It's not much different, just more tedious. :-)

You get Symbol.toStringTag with v8::Symbol::GetToStringTag(isolate) and set it on the v8::FunctionTemplate::PrototypeTemplate() of your constructor callback.

@nschonni
Copy link
Member Author

Thanks for the hints! I'll see what I can figure out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. wontfix Issues that will not be fixed.
Projects
None yet
Development

No branches or pull requests

2 participants