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

Add support for profiling rule performance via the TIMING environment variable #8108

Merged

Conversation

ryo-manba
Copy link
Member

@ryo-manba ryo-manba commented Nov 4, 2024

Which issue, if any, is this issue related to?

Closes #7350

Is there anything in the PR that needs further explanation?

Added support for TIMING environment variable to track rule performance, similar to ESLint's profiling feature.
Set TIMING=1 to display the top 10 longest-running rules or TIMING=all to display all rules.

demo

TIMING=1

EDIT: The specification was modified, and only a single line is displayed when TIMING=1.

image

TIMING=all

image

Copy link

changeset-bot bot commented Nov 4, 2024

🦋 Changeset detected

Latest commit: 8b83074

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Mouvedia
Copy link
Member

Mouvedia commented Nov 4, 2024

This looks cool.

Normally you ask before taking a status: ask to implement issue.
That's for your own sake, having to scrap your PR because things have changed would be disheartening; trust me Iv been there.
Anyway since it has 3 👍 and @jeddy3 is the one who accepted it Ill switch it to status: ready to implement.

lib/timing.mjs Outdated Show resolved Hide resolved
lib/lintPostcssResult.mjs Outdated Show resolved Hide resolved
lib/timing.mjs Outdated Show resolved Hide resolved
lib/timing.mjs Outdated Show resolved Hide resolved
ryo-manba and others added 2 commits November 14, 2024 22:05
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
lib/timing.mjs Outdated Show resolved Hide resolved
ryo-manba and others added 4 commits November 22, 2024 00:02
@ryo-manba ryo-manba requested a review from Mouvedia November 30, 2024 01:05
docs/user-guide/cli.md Outdated Show resolved Hide resolved
Co-authored-by: Gary Gozlan <Mouvedia@users.noreply.github.com>
lib/timing.mjs Outdated Show resolved Hide resolved
@ryo-manba

This comment was marked as resolved.

@ybiquitous

This comment was marked as resolved.

@ryo-manba

This comment was marked as resolved.

@ybiquitous
Copy link
Member

I'm reconsidering the TIMING environment variable. If we think of the possibility of conflict with the environment variable (e.g., other tools may use the variable), it seems to make sense to provide a CLI flag like --timing instead. Any thoughts? 🤔

@Mouvedia
Copy link
Member

If you have a separate npm script for each type of lint it shouldn't matter.
i.e. only having to prefix one environment variable to "lint:*" could be considered a convenience (if we share the same default for true/1)

Any thoughts?

I think that it doesn't matter.
i.e. I don't have a preference for either one

But with the flag we will have to start discussing whether we add its configuration property and Node.js option counterparts.
The environment variable doesn't have this problem.

@ybiquitous
Copy link
Member

But with the flag we will have to start discussing whether we add its configuration property and Node.js option counterparts.
The environment variable doesn't have this problem.

Make sense. Agree with choosing the TIMING environment variable 👍🏼

@Mouvedia
Copy link
Member

This is not to be implemented in this PR but I gotta ask, what about supporting TIMING=rule-name?
e.g. TIMING=no-descending-specificity

@ybiquitous
Copy link
Member

what about supporting TIMING=rule-name?

That sounds good, although it should not be a blocker of this PR 👍🏼

Probably, supporting multiple rules would be fine, e.g., TIMING=rule-1, rule-2.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Overall LGTM, but I've left a few suggestions to improve the code maintainability.

lib/timing.mjs Outdated Show resolved Hide resolved
lib/timing.mjs Outdated Show resolved Hide resolved
lib/timing.mjs Outdated Show resolved Hide resolved
lib/timing.mjs Show resolved Hide resolved
@ryo-manba ryo-manba requested a review from ybiquitous December 15, 2024 15:15
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks LGTM 👍🏼

I probably recommend asking @jeddy3 to review it again 😃

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM.

@ryo-manba, thank you for persevering with the requested changes. The feature is better because of them.

@Mouvedia and @ybiquitous, thank you for your attention to detail regarding code quality. Your efforts here and in other PRs have benefited the entire codebase!

@jeddy3 jeddy3 changed the title Add support for profiling rule performance Add support for profiling rule performance via the TIMING environment variable Dec 16, 2024
@jeddy3 jeddy3 merged commit 7a74b91 into stylelint:main Dec 16, 2024
16 of 17 checks passed
@ryo-manba
Copy link
Member Author

ryo-manba commented Dec 16, 2024

@ybiquitous
Thanks a lot for your thoughtful review!

@jeddy3
I would be happy if you could review this when you have time.

Edit:
Oh, it seems I didn't update it. It's already been merged 😅

@ryo-manba ryo-manba requested a review from jeddy3 December 16, 2024 12:40
@jeddy3 jeddy3 removed request for Mouvedia and jeddy3 December 16, 2024 12:42
@ryo-manba ryo-manba deleted the feat/add-support-for-profiling-rule-performance branch December 16, 2024 12:42
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Jan 12, 2025
| datasource | package   | from    | to      |
| ---------- | --------- | ------- | ------- |
| npm        | stylelint | 16.12.0 | 16.13.0 |


## [v16.13.0](https://github.com/stylelint/stylelint/blob/HEAD/CHANGELOG.md#16130---2025-01-12)

It adds 3 rules to help you write error-free at-rules and 2 rules to warn you about deprecated CSS features. We've turned these rules on in our [standard config](https://www.npmjs.com/package/stylelint-config-standard).

It also adds new rule options, a feature to display how long rules take, lax autofix and support for `messageArgs` in more rules. It fixes 7 bugs.

Lastly, we've made a deprecation that may affect some plugins. We've updated our docs for [plugin authors](docs/developer-guide/plugins.md#quiet-deprecation-warnings) and [end users](docs/user-guide/options.md#quietdeprecationwarnings) on how to silence deprecation warnings.

-   Deprecated: ambiguous position arguments passed to `utils.report()` ([#8244](stylelint/stylelint#8244)) ([@romainmenke](https://github.com/romainmenke)).
-   Added: `lax`/`strict` values to the `fix` Node.js API option and CLI flag ([#8106](stylelint/stylelint#8106)) ([@ryo-manba](https://github.com/ryo-manba)).
-   Added: support for profiling rule performance via the `TIMING` environment variable ([#8108](stylelint/stylelint#8108)) ([@ryo-manba](https://github.com/ryo-manba)).
-   Added: `at-rule-descriptor-no-unknown` rule ([#8197](stylelint/stylelint#8197)) ([@ryo-manba](https://github.com/ryo-manba)).
-   Added: `at-rule-descriptor-value-no-unknown` rule ([#8211](stylelint/stylelint#8211)) ([@ryo-manba](https://github.com/ryo-manba)).
-   Added: `at-rule-no-deprecated` rule ([#8251](stylelint/stylelint#8251)) ([@jeddy3](https://github.com/jeddy3)).
-   Added: `at-rule-prelude-no-invalid` rule ([#8268](stylelint/stylelint#8268)) ([@ryo-manba](https://github.com/ryo-manba)).
-   Added: `declaration-property-value-keyword-no-deprecated` rule ([#8223](stylelint/stylelint#8223)) ([@Mouvedia](https://github.com/Mouvedia)).
-   Added: `"ignore": ["at-rule-preludes", "declaration-values"]` to `string-no-newline` ([#8214](stylelint/stylelint#8214)) ([@ryo-manba](https://github.com/ryo-manba)).
-   Added: `messageArgs` to `declaration-property-value-no-unknown`, `font-family-name-quotes`, `font-family-no-duplicate-names`, `function-calc-no-unspaced-operator`, `import-notation`, `media-feature-name-unit-allowed-list`, `selector-attribute-quotes` and `selector-pseudo-element-colon-notation` ([#8285](stylelint/stylelint#8285) & [#8252](stylelint/stylelint#8252)) ([@Mouvedia](https://github.com/Mouvedia)).
-   Fixed: deprecation warnings to only display once per (custom) rule ([#8265](stylelint/stylelint#8265)) ([@romainmenke](https://github.com/romainmenke)).
-   Fixed: `*-no-vendor-prefix` message ambiguity ([#8239](stylelint/stylelint#8239)) ([@Mouvedia](https://github.com/Mouvedia)).
-   Fixed: `at-rule-(dis)allowed-list`, `at-rule-no-vendor-prefix`, `at-rule-property-required-list` message argument ([#8277](stylelint/stylelint#8277)) ([@Mouvedia](https://github.com/Mouvedia)).
-   Fixed: `at-rule-property-required-list` message for inclusion of properties and descriptors ([#8207](stylelint/stylelint#8207)) ([@jeddy3](https://github.com/jeddy3)).
-   Fixed: `at-rule-*` false positives and negatives for `@charset` rule ([#8215](stylelint/stylelint#8215)) ([@jeddy3](https://github.com/jeddy3)).
-   Fixed: `declaration-property-value-no-unknown` false positives for descriptors ([#8240](stylelint/stylelint#8240)) ([@jeddy3](https://github.com/jeddy3)).
-   Fixed: `property-(dis)allowed-list` false negatives for custom properties, use `/^--/` to (dis)allow them ([#8209](stylelint/stylelint#8209)) ([@fbasmaison-lucca](https://github.com/fbasmaison-lucca)).
-   Fixed: `property-no-unknown` false positives for descriptors ([#8203](stylelint/stylelint#8203)) ([@jeddy3](https://github.com/jeddy3)).
-   Fixed: `selector-pseudo-class-no-unknown` false positives for deprecated pseudo-classes ([#8264](stylelint/stylelint#8264)) ([@Mouvedia](https://github.com/Mouvedia)).
-   Fixed: `selector-type-case` false positives for `hatchPath` ([#8264](stylelint/stylelint#8264)) ([@Mouvedia](https://github.com/Mouvedia)).
-   Fixed: `selector-type-no-unknown` false positives for `shadow`, `hatch` and `hatchpath` ([#8264](stylelint/stylelint#8264)) ([@Mouvedia](https://github.com/Mouvedia)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add support for profiling rule performance
4 participants