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

feat: log complete config in debug mode #18289

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

btea
Copy link
Collaborator

@btea btea commented Oct 5, 2024

Description

When I set the debug parameters and prepare to view relevant information, I expect to be able to see some object information with a deeper nesting level more intuitively.

before after
image image

Copy link

stackblitz bot commented Oct 5, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy
Copy link
Member

bluwy commented Oct 5, 2024

Could it be set for the config loading specifically only? I'm not sure if Vite should change the default depth as that might expand other logs.

@btea
Copy link
Collaborator Author

btea commented Oct 5, 2024

That makes sense, it seems that adding a new configuration alone will have the least impact.

packages/vite/src/node/config.ts Outdated Show resolved Hide resolved
@btea btea changed the title chore: set the DEBUG_DEPTH variable feat: add debugDepth option Oct 6, 2024
@btea
Copy link
Collaborator Author

btea commented Oct 17, 2024

Maybe we can add a debugDepth option like the cli option debug.

@bluwy
Copy link
Member

bluwy commented Oct 25, 2024

Sorry for not responding before. I was thinking the depth could be more of an option for createDebugger() options, and we can set the value on log.inspectOpts.depth so that the depth is scoped to the logging directly.

Great find with log.inspectOpts.depth though. I didn't know and thought we'd hit a brick wall. So we could do a change like this:

export function createDebugger(
  namespace: ViteDebugScope,
  options: DebuggerOptions = {},
): debug.Debugger['log'] | undefined {
  const log = debug(namespace)
  if (options.depth && log.inspectOpts.depth == null) {
    log.inspectOpts.depth = options.depth
  }

What do you think?

@btea
Copy link
Collaborator Author

btea commented Oct 26, 2024

This looks good. However, I have a question. createDebugger is a global public function. We can't specify whether it should be applied to a certain scope through parameter configuration.

@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

I'm not quite sure what you mean. Doesn't my code example only applies the custom depth to an instance of debug()? So only calling the returned log() will have the special depth behaviour, but other debug log() will not have, so it's not polluted elsewhere.

@btea
Copy link
Collaborator Author

btea commented Oct 28, 2024

What I mean is that all debug logs that are output now are created by createDebugger. If I pass a configuration parameter in like I currently do, it seems that I can't determine which call createDebugger to pass in the second parameter.

@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

Oh, what I was thinking is that we don't need to have a new debugDepth config in Vite. If the user wants to configure that, I think it's enough to use DEBUG_DEPTH=100 before running Vite.

I think it should be enough for Vite to pass an opinionated:

const debug = createDebugger('vite:config', { depth: 100 })

So that it gets a nice logging by default, only for the vite:config namespace.

@btea
Copy link
Collaborator Author

btea commented Oct 28, 2024

I think it's enough to use DEBUG_DEPTH=100 before running Vite.

Directly configure the environment variable DEBUG_DEPTH=100? Or is it to configure a cli option? If we configure the environment variable directly, then debug will handle it directly, and the following manual passing of the second parameter should be unnecessary.

@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

Directly configure the environment variable. The issue though is that because it defaults to 2 (via nodes formatWithOptions), it's not the best default to use for config logging. We could change the default for the specific logging instead to eg 10 so that it logs better without any config. But if the user explicitly set DEBUG_DEPTH we can respect that instead.

@btea
Copy link
Collaborator Author

btea commented Oct 28, 2024

Oh, I see. However, changing this default value will cause the default output information structure to change. Will it affect some users?

@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

I think since it's only debug information, it should probably be safe. They should also get more logging of the config by default which I think is better.

@btea
Copy link
Collaborator Author

btea commented Oct 28, 2024

OK, I have updated it.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks! Let's see what others think of this change.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I like this!

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Oct 28, 2024
@patak-dev patak-dev changed the title feat: add debugDepth option feat: log complete config in debug mode Oct 30, 2024
@bluwy bluwy merged commit 04f6736 into vitejs:main Oct 30, 2024
16 checks passed
@btea btea deleted the chore/config-debug-depth-variable branch October 30, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants