zlib: use common owner symbol to access JS wrapper #23189
Conversation
Use the same symbol that other `AsyncWrap` instances also use for accessing the JS wrapper.
| @@ -300,7 +301,8 @@ function Zlib(opts, mode) { | |||
| Transform.call(this, opts); | |||
| this.bytesWritten = 0; | |||
| this._handle = new binding.Zlib(mode); | |||
| this._handle.jsref = this; // Used by processCallback() and zlibOnError() | |||
| // Used by processCallback() and zlibOnError() | |||
| this._handle[owner_symbol] = this; | |||
There was a problem hiding this comment.
Maybe use a non-enumerable property here? Or would this not show up when inspecting the object?
There was a problem hiding this comment.
It would, but it would show up only when inspecting the handle object. In that case, I think pointing to the JS wrapper is a good idea.
When inspecting the JS wrapper, this shows up as [Circular], which seems okay to me.
There was a problem hiding this comment.
I personally would rather hide it since the property itself is probably not useful for the regular user. I also prefer to hide such things so less users would try to abuse any internals. If someone wants to see these properties it's always possible by explicitly requesting that.
There was a problem hiding this comment.
In that case _handle is probably the thing to hide, not the owner on the handle.
One thing I know I’ve thought about a while ago is a custom inspection function for all native handles – not something that blends them out completely, but rather gives a useful overview over the current state in some way.
addaleax
added
the
author ready
|
FreeBSD again: https://ci.nodejs.org/job/node-test-commit-freebsd/21012/ |
|
Landed in a9bb653 |
Use the same symbol that other `AsyncWrap` instances also use for accessing the JS wrapper. PR-URL: #23189 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Use the same symbol that other `AsyncWrap` instances also use for accessing the JS wrapper. PR-URL: #23189 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Use the same symbol that other `AsyncWrap` instances also use for accessing the JS wrapper. PR-URL: #23189 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Use the same symbol that other
AsyncWrapinstances also usefor accessing the JS wrapper.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes