-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Improve debug logs #15303
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
Improve debug logs #15303
Conversation
776e646 to
0430e9d
Compare
| } | ||
|
|
||
| export async function handle(args: Result<ReturnType<typeof options>>) { | ||
| using I = new Instrumentation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this is only created when DEBUG is true but I don't think that's possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about this feature 😮 I think it's cool but haven't really used this before. Is there any way to only conditionally create an instance of Instrumentation? 🤔
I'm not sure yet what to think about it, probably just takes some getting used to haha. It seems like it's the same as defer in Go but the fact that it requires an object with a dispose symbol seems a little extra verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the compiled output (because we can't use the native version just yet), what it essentially does is it wraps the code in a try/catch/finally and calls the dispose function in the finally block.
The cool part is that you don't have to worry about all the spots where you used an early return to flush the changes to the terminal because the moment you return, the finally block is executed as well.
But yeah, I don't know/think you can create one conditionally because of this try/catch setup 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine. It's like a few objects and closures being created. The overhead for this is massively dwarfed by everything else. And reporting doesn't happen unless in debug mode either so that code does nothing.
|
|
||
| static SHOULD_TRACE: sync::LazyLock<bool> = sync::LazyLock::new( | ||
| || matches!(std::env::var("DEBUG"), Ok(value) if value.eq("*") || value.eq("1") || value.eq("true") || value.contains("tailwind")), | ||
| || matches!(std::env::var("DEBUG"), Ok(value) if value.eq("*") || (value.contains("tailwindcss:oxide") && !value.contains("-tailwindcss:oxide"))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using DEBUG=1 it also shows a lot of oxide related tracing logs. This change only shows them if you are using DEBUG=* or DEBUG=tailwindcss:oxide by following the DEBUG conventions.
|
|
||
| // Scan the directory for candidates | ||
| env.DEBUG && console.time('[@tailwindcss/cli] Scan for candidates') | ||
| DEBUG && I.start('Scan for candidates') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are called after the initial I.start('[@tailwincss/cli]'), this allows us to remove the prefix because these logs will be indented.
| DEBUG && I.start('Optimization') | ||
|
|
||
| DEBUG && I.start('AST -> CSS') | ||
| let css = toCss(tailwindCssAst) | ||
| DEBUG && I.end('AST -> CSS') | ||
|
|
||
| DEBUG && I.start('Lightning CSS') | ||
| let ast = optimizeCss(css, { | ||
| minify: typeof optimize === 'object' ? optimize.minify : true, | ||
| }) | ||
| DEBUG && I.end('Lightning CSS') | ||
|
|
||
| DEBUG && I.start('CSS -> PostCSS AST') | ||
| context.optimizedPostCssAst = postcss.parse(ast, result.opts) | ||
| DEBUG && I.end('CSS -> PostCSS AST') | ||
|
|
||
| DEBUG && I.end('Optimization') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, but split up explicitly
| "target": "es2022", | ||
| "lib": ["es2022", "esnext.disposable", "dom"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are needed to compile the new using keyword
0049ab7 to
ab99449
Compare
410bb2e to
e317a2c
Compare
If we bail early, then a `console.time('…')` was already started and
subsequent calls to `console.timeEnd('…')` will fail because the
`console.time` and `console.timeEnd` don't line up.
This also allows us to make it disposable. This allows us to create a new instrumentation instance using the `using` keyword, which automatically reports the findings when it's being disposed.
This is only the case for when we are automatically disposing the `Instrumentation`.
This prevents `env.DEBUG` property access over and over again
e317a2c to
d9aa08d
Compare
thecrypticace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay
We recently introduced some better instrumentation (#15303) which uses the new `using` keyword. I made sure that this was compiled correctly for environments where `using` is not available yet. The issue is that this also relies on `Symbol.dispose` being available. In my testing on our minimal required Node.js version (18) it did work fine. However, turns out that I was using `18.20.x` locally where `Symbol.dispose` **_is_** available, but on older version of Node.js 18 (e.g.: `18.17.x`) it is **_not_** available. This now results in some completely broken builds, e.g.: when running on Cloudflare Pages. See: #15399 I could reproduce this error in CI, by temporarily downgrading the used Node.js version to `18.17.0`. See: <img width="1142" alt="image" src="https://github.com/user-attachments/assets/5bf30f80-9ca0-40d9-ad02-d1ffb4e0e5dd" /> Implementing the proper polyfill, as recommended by the TypeScript docs ( see: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html#:~:text=Symbol.dispose,-??=%20Symbol(%22Symbol.dispose ), the error goes away. (If you look at CI after the polyfill, it still fails but for different reasons unrelated to this change) Fixes: #15399 --- ## Test plan 1. I reproduced it in CI, and I kept the commits so that you can take a look where it fails with the `Object not disposable`. 2. Using the provided reproduction from #15399: ### Before It works on Node.js v18.20.x, but switching to Node.js v18.17.x you can see it fail: <img width="1607" alt="image" src="https://github.com/user-attachments/assets/cb6ab73a-8eb2-4003-bab7-b2390f1c879d" /> ### After Using pnpm's overrides, we can apply the fix from this PR and test it in the reproduction. You'll notice that it now works in both Node.js v18.20.x and v18.17.x <img width="1604" alt="image" src="https://github.com/user-attachments/assets/b3a65557-0658-4cb0-a2f9-e3079c7936d5" />
This PR improves the debug logs for the
@tailwindcss/postcssintegration. It uses custom instrumentation to provide a nested but detailed overview of where time is spent during the build process.The updated logs look like this: