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: use debug package for debug logging #3167

Open
wants to merge 6 commits into
base: v2
Choose a base branch
from

Conversation

KermanX
Copy link

@KermanX KermanX commented Jun 28, 2024

Previously, !!process.env.DEBUG was used to check whether to debug in @typescript/vfs and @typescript/twoslash. However, most of the packages are using the debug package to debug. When we set DEBUG=vite:hmr, @typescript/vfs will output lots of things, which immediately fill the terminal and prevent us from watching logs from Vite.

Also, the debug package has been installed as a dependency in @typescript/vfs, but seems unused:

And the README also said it's using the debug package:

- All packages use [debug](https://www.npmjs.com/package/debug) - which means you can do `env DEBUG="*" pnpm test` to get verbose logs

@jakebailey
Copy link
Member

CI will fail here because this change is missing a changeset.

@jakebailey jakebailey added the deploy-preview Enables automatic deployments to preview environments on a PR label Jun 29, 2024
@@ -49,6 +49,7 @@
},
"devDependencies": {
"@rollup/plugin-commonjs": "^25.0.7",
"@types/debug": "^4.1.12",
Copy link
Member

Choose a reason for hiding this comment

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

This seems to imply that sandbox needs a dep on debug; I think that's true given we vendor the vfs into this package?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. The DTS build of @typescript/sandbox would fail unless @types/debug is installed, because @typescript/vfs is vendored.

Copy link
Member

Choose a reason for hiding this comment

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

Right but I mean that this package should also depend on "debug" too so the code doesn't crash.

@jakebailey
Copy link
Member

Checking out the vendored code, it does now import from debug. I'm not 100% certain if that's desirable, though; I believe this code is intended to be loaded in the browser directly so this code may break there now that it's not dependency free.

What confuses me though is why lzstring is bundled and not debug. I guess we locally vendor lzstring? Maybe we need to tweak our config to ensure that these packages are still self contained?

@orta do you recall the restrictions here?

Copy link
Contributor

github-actions bot commented Jul 9, 2024

Azure Static Web Apps: Your stage site is ready! Visit it here: https://victorious-plant-05c166c10-3167.centralus.5.azurestaticapps.net

@orta
Copy link
Contributor

orta commented Jul 9, 2024

I just didn't think that debug was important enough of a dependency to add it to the playground's web bundle - given how little I was using of it, could replicate the check for the substring in this package to copy the behavior they are looking for?

@jakebailey
Copy link
Member

The debug module does a lot of stuff, especially on web; I'm not sure it would be exactly trivial to copy that bit.

But, I think maybe we could just change the original code to check DEBUG=typescript:vfs (and similar) instead, and just not document that we use the debug module?

(sorry for this reversal, I just looked at the output and noticed that this would break someone)

@orta
Copy link
Contributor

orta commented Jul 10, 2024

Yeah, I think that's the right call

@jakebailey jakebailey removed the deploy-preview Enables automatic deployments to preview environments on a PR label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants