util: support inspecting namespaces of unevaluated modules#20782
util: support inspecting namespaces of unevaluated modules#20782devsnek wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
the
util
lib/util.js
Outdated
There was a problem hiding this comment.
Can you s/ns/value/? All functions call it that except this one.
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Great to see these debugging experiences made nicer.
lib/util.js
Outdated
There was a problem hiding this comment.
This line should rather go along with the value checks at line 571.
There was a problem hiding this comment.
i'm not quite sure how to do that, should i make another formatObject function like formatNamespaceObject?
There was a problem hiding this comment.
Yes, that would be the ideal way here. That way there is no performance impact on regular objects.
There was a problem hiding this comment.
i'll move it for consistency, but just for the record the isModuleNamespaceObject check still happens either way, there is no perf difference.
There was a problem hiding this comment.
I am not sure how V8 optimizes the loop in this case. I guess it could be smart to remove the isNs check but otherwise there would be a minor overhead.
lib/util.js
Outdated
There was a problem hiding this comment.
array === 0 will always be true.
lib/util.js
Outdated
There was a problem hiding this comment.
I am somewhat surprised that the keys are exposed even though accessing them throws. I guess that is done by design - out of curiosity: would you be so kind and point me to the reasons why this is done like that? After looking at the test again, I see why it throws.
There was a problem hiding this comment.
even though we know the export names, they aren't yet intialised (think console.log(a); const a = 1). there is no js value that can represent this state so v8 throws a reference error, which is what would happen with that example.
lib/util.js
Outdated
There was a problem hiding this comment.
Not a fan of copy pasting this part. It could easily get out of sync if anything small changes.
|
@addaleax thanks for starting the benchmark. If they turn out to have a impact I would like to only show this when |
devsnek
force-pushed
the
fix/util-inspect-module-namespace
branch
from
83ed8df to
f0644e4
Compare
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM with my nits addressed.
lib/util.js
Outdated
There was a problem hiding this comment.
It would be nice to move this down to the last else because it is probably one of the least used values and does not need to be performant.
lib/util.js
Outdated
There was a problem hiding this comment.
Please do not use a default value and just explicitly pass through the value.
lib/util.js
Outdated
There was a problem hiding this comment.
This could now be rewritten to: if (foo) return ... if (bar) return ...
devsnek
force-pushed
the
fix/util-inspect-module-namespace
branch
from
f0644e4 to
d065be3
Compare
|
linuxone rebuild: https://ci.nodejs.org/job/node-test-commit-linuxone/1372/ failures look unrelated |
devsnek
added
the
author ready
|
landed in 064057b |
devsnek
removed
the
author ready
PR-URL: #20782 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #20782 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes