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

chore: centralize debug codes #4446

Closed
wants to merge 1 commit into from
Closed

Conversation

benmccann
Copy link
Collaborator

Description

Centralize debug codes in one place and add code comment documenting their usage

Additional context

I had a hard time figuring out how to turn on debug logs. By putting them all in one place and adding a code comment explaining how to use them it should make it easier to get started using them


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@benmccann benmccann force-pushed the debug-codes branch 4 times, most recently from 7f1a393 to 6d322f5 Compare July 30, 2021 04:35
packages/vite/src/node/debugger.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added the p1-chore Doesn't change code behavior (priority) label Jul 30, 2021
@antfu
Copy link
Member

antfu commented Jul 30, 2021

To be honest, I don't see much value of introducing this change, but might introduce additional fiction to developers to debug and making changes.

@Shinigami92
Copy link
Member

Shinigami92 commented Jul 30, 2021

additional fiction

What do you mean by additional fiction?


Beside that I think it would be nice if we document the different CLI debug filters (we can talk about this here https://discord.com/channels/804011606160703521/870545448618819626/870545450376261662, and/or in another PR)
See #4423 (comment)

@antfu
Copy link
Member

antfu commented Jul 30, 2021

Let's imagine the scenario when will someone want do use the debugger of Vite - they are mostly trying to find the internal bug of vite, or trying to understand how vite work. Either way, they should already reading the source code of vite. Before this change, each usage of debug is definite inside their corresponding module, when you trying to debug the module you can easily pick up the debug code and try it in the console (which is pretty good self-documenting to me). Now with centralized codes, you will need to jump to the file to see it (not so hard on VS Code, but how about GitHub or some other editors?)

The additional fiction I mean:

  • Multiple files instead of one for understanding the context
  • Extra import statement, which you will also need to deal with the relative paths
  • Most of the codes are only been used once
  • When you want to add another logger, you will need to update the codes and import it to use.
  • 'vite:config' -> DebugScopes.CONFIG more characters to type?
  • I don't think it's hard to keep the vite: convention manually (as it won't be touched frequently either)
  • It was self-documenting, I don't think it's worth to break it and document separately.

I would feel this is a bit over-engineering tbh.

@Shinigami92
Copy link
Member

Let's imagine the scenario when will someone want do use the debugger of Vite - they are mostly trying to find the internal bug of vite, or trying to understand how vite work. Either way, they should already reading the source code of vite. Before this change, each usage of debug is definite inside their corresponding module, when you trying to debug the module you can easily pick up the debug code and try it in the console (which is pretty good self-documenting to me). Now with centralized codes, you will need to jump to the file to see it (not so hard on VS Code, but how about GitHub or some other editors?)

The additional fiction I mean:

  • Multiple files instead of one for understanding the context
  • Extra import statement, which you will also need to deal with the relative paths
  • Most of the codes are only been used once
  • When you want to add another logger, you will need to update the codes and import it to use.
  • 'vite:config' -> DebugScopes.CONFIG more characters to type?
  • I don't think it's hard to keep the vite: convention manually (as it won't be touched frequently either)
  • It was self-documenting, I don't think it's worth to break it and document separately.

I would feel this is a bit over-engineering tbh.

That are some good points from @antfu ...

But I'm not going with all of your points.


  • Multiple files instead of one for understanding the context

This is not even bad but just clean up the always getting bigger and bigger project.
Now the debugging stuff can be placed and live in one maintained file.


  • Extra import statement, which you will also need to deal with the relative paths

I never had problems with importing stuff from different files. VSCode always helped me and even organized them. So I never really care about imports.


  • Most of the codes are only been used once

That is a valid and true point.


  • When you want to add another logger, you will need to update the codes and import it to use.

Yeah, that's the whole point of bringing everything together in one place instead of leaving everything lying around in different places. With that you can now jump through the (code in your IDE, or in GitHub source code view, sadly not the review panels) and explore all places that contains debugging stuff.


  • 'vite:config' -> DebugScopes.CONFIG more characters to type?
  1. Code is also for humans cause they need to read it. Let me directly say that DebugScopes.CONFIG directly makes clear to me what it is doing.
  2. Typing 'vite:config' are 11 chars plus 2 quotes for me, where as DebugScopes.CONFIG is a D -> CTRL+Space -> . -> C -> CTRL+Space that sum up in 5 (7 if you count CTRL+Space as 2) keystrokes. So, sorry but I cannot accept this argument.

  • I don't think it's hard to keep the vite: convention manually (as it won't be touched frequently either)

Sure, that's not necessary if wanted. But not part of this PR.


  • It was self-documenting, I don't think it's worth to break it and document separately.

I think it's worth to be clearly documented, cause not everyone (even me before this PR!) knows that you can debug with that way. The user documentation doesn't say anything about this debug possibility.
When searching for debug the result doesn't help that much:

[...] they are mostly trying to find the internal bug of vite [...]

I don't think so, I wasn't aware myself that you can e.g. debug like that + it's even possible to use patterns like 'vite:*'.
If you don't know that from the docs, you struggle and waste some times by trying around and hopefully find out that vite has this opportunity. Why not make users directly clear and help them out of the box and therefore help us by e.g. raise fewer issues and discussions.

And yeah you can also filter by yarn vite --debug "time". But you need to find out by using the CLI help + need to find out what even a [feat] is, and in addition you need to know that you have to use e.g. "time" instead of "vite:time". So the logic seems also to be different 🤔


To sum up what I want to achieve is improved user documentation for DEBUG mode.
Even a list of all features and how the DEBUG env variable differs from the CLI command and that it can be used for e.g. advanced patterns could be useful.
The code in this PR helps to get a bit better clear view of what's possible. But in the end it shows me that we have a real big lack of user documentation in this field 👀

@patak-dev
Copy link
Member

Given that we don't get type safety as people can still just pass in a string, the extra import looks like too much for what we get. I don't see much of a difference in DX by centralizing things in the code instead of in the documentation. I totally agree with you about a big hole in the docs here. @ElMassimo wrote a post about this https://maximomussini.com/posts/debugging-javascript-libraries/ that fills in some of these gaps and the idea was to maybe add something similar in an advanced section in the docs.
FWIW, at this point, my vote goes to leave code as is, and document how this works and we can also have a list of the most important with a description (just the name is not very useful either). So the process would be:

  1. start using new codes as they are needed without extra requirement
  2. once the pattern is settled and it proves useful add it to the list of public debug codes

@benmccann benmccann mentioned this pull request Aug 1, 2021
9 tasks
@benmccann benmccann closed this Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants