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.

Sign up for GitHub

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

Jump to bottom

util: support inspecting namespaces of unevaluated modules #20782

Closed
wants to merge 1 commit into from
Closed

util: support inspecting namespaces of unevaluated modules #20782

wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented May 16, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label May 16, 2018
bnoordhuis
bnoordhuis approved these changes May 16, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo nit.

lib/util.js Outdated
@@ -993,6 +997,29 @@ function formatPromise(ctx, value, recurseTimes, keys) {
return output;
}

function formatNamespaceProperty(ctx, ns, recurseTimes, key, array) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you s/ns/value/? All functions call it that except this one.

guybedford
guybedford requested changes May 16, 2018
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great to see these debugging experiences made nicer.

lib/util.js Outdated
@@ -761,8 +761,12 @@ function formatError(value) {
function formatObject(ctx, value, recurseTimes, keys) {
const len = keys.length;
const output = new Array(len);
for (var i = 0; i < len; i++)
output[i] = formatProperty(ctx, value, recurseTimes, keys[i], 0);
const isNs = types.isModuleNamespaceObject(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should rather go along with the value checks at line 571.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not quite sure how to do that, should i make another formatObject function like formatNamespaceObject?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be the ideal way here. That way there is no performance impact on regular objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll move it for consistency, but just for the record the isModuleNamespaceObject check still happens either way, there is no perf difference.

Copy link
Member

Choose a reason for hiding this comment

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

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.

BridgeAR
BridgeAR reviewed May 16, 2018
View reviewed changes
lib/util.js Outdated
let name;
let value;
try {
value = formatValue(ctx, ns[key], recurseTimes, array === 0);
Copy link
Member

Choose a reason for hiding this comment

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

array === 0 will always be true.

lib/util.js Outdated
value = formatValue(ctx, ns[key], recurseTimes, array === 0);
} catch (err) {
if (err instanceof ReferenceError) {
value = ctx.stylize('<uninitialized>', 'special');
Copy link
Member

@BridgeAR BridgeAR May 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
name = ctx.stylize(strEscape(key), 'string');
}

return `${name}: ${value}`;
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of copy pasting this part. It could easily get out of sync if anything small changes.

@addaleax
Copy link
Member

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/194/
CI: https://ci.nodejs.org/job/node-test-pull-request/14910/

@BridgeAR
Copy link
Member

@addaleax thanks for starting the benchmark.

If they turn out to have a impact I would like to only show this when showHidden is active.

@devsnek devsnek force-pushed the fix/util-inspect-module-namespace branch from 83ed8df to f0644e4 Compare May 16, 2018 19:34
BridgeAR
BridgeAR approved these changes May 17, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM with my nits addressed.

lib/util.js Outdated
@@ -589,6 +589,9 @@ function formatValue(ctx, value, recurseTimes, ln) {
if (keyLength === 0)
return ctx.stylize(`[${name}]`, 'special');
base = `[${name}]`;
} else if (types.isModuleNamespaceObject(value)) {
braces[0] = `[${tag}] {`;
formatter = formatNamespaceObject;
Copy link
Member

Choose a reason for hiding this comment

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

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
@@ -993,8 +1005,37 @@ function formatPromise(ctx, value, recurseTimes, keys) {
return output;
}

function formatKey(ctx, key, enumerable = true) {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use a default value and just explicitly pass through the value.

lib/util.js Outdated
} else {
name = ctx.stylize(strEscape(key), 'string');
}
return name;
Copy link
Member

Choose a reason for hiding this comment

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

This could now be rewritten to: if (foo) return ... if (bar) return ...

@devsnek devsnek force-pushed the fix/util-inspect-module-namespace branch from f0644e4 to d065be3 Compare May 17, 2018 16:07
@devsnek
Copy link
Member Author

devsnek commented May 17, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/14934/

@devsnek
Copy link
Member Author

devsnek commented May 17, 2018

linuxone rebuild: https://ci.nodejs.org/job/node-test-commit-linuxone/1372/
freebsd rebuild: https://ci.nodejs.org/job/node-test-commit-freebsd/17840/
linux rebuild: https://ci.nodejs.org/job/node-test-commit-linux/18842/

failures look unrelated

@devsnek devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018
@devsnek
Copy link
Member Author

devsnek commented May 19, 2018

landed in 064057b

@devsnek devsnek closed this May 19, 2018
@devsnek devsnek deleted the fix/util-inspect-module-namespace branch May 19, 2018 01:41
@devsnek devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2018
devsnek added a commit that referenced this pull request May 19, 2018
@devsnek
util: support inspecting namespaces of unevaluated modules
064057b 
PR-URL: #20782
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@devsnek @MylesBorins
util: support inspecting namespaces of unevaluated modules
b308a07 
PR-URL: #20782
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@guybedford guybedford guybedford approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@devsnek @addaleax @BridgeAR @bnoordhuis @jasnell @guybedford @nodejs-github-bot