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: Support Stylelint custom syntax object #937

Merged
merged 7 commits into from
Feb 12, 2025

Conversation

TJNhxMZHmqGytuWT
Copy link
Contributor

@TJNhxMZHmqGytuWT TJNhxMZHmqGytuWT commented Feb 8, 2025

The customSyntax option in Stylelint config can accept a module name, but it can also accept a Syntax object:

// .stylelintrc.js
module.exports = {
  customSyntax: require('postcss-less'),
};

Some examples in the wild:
https://github.com/search?q=%22customSyntax%3A+require%28%22&type=code

Since Knip assumes that config.customSyntax can only be a string, running it with this kind of config results in the following error:

export const isInNodeModules = (filePath) => filePath.includes('node_modules');
                                                      ^
TypeError: filePath.includes is not a function

I updated the Stylelint plugin to avoid trying to resolve syntax objects.

I also added tests for JS config files.

@TJNhxMZHmqGytuWT TJNhxMZHmqGytuWT marked this pull request as ready for review February 8, 2025 14:20
Copy link

pkg-pr-new bot commented Feb 10, 2025

Open in Stackblitz

npm i https://pkg.pr.new/knip@937

commit: 9178bb0

Copy link
Collaborator

@webpro webpro 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 PR! Happy to merge, but please first see the feedback.

assert(issues.dependencies['package.json']['stylelint-order']);
assert(issues.unresolved['stylelint.config.js']['stylelint-config-standard']);
assert(issues.unlisted['stylelint.config.js']['stylelint-config-recommended']);
assert(issues.unresolved['.stylelintrc.mjs']['stylelint-config-standard']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea of most of the plugin tests is to not leave any issues, so only the processed and total counters should not be zero (0). In other words, the dependencies referenced in the plugin config files in the fixtures should also be listed in package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! Thanks

Copy link
Collaborator

@webpro webpro left a comment

Choose a reason for hiding this comment

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

Thanks!

Idea: if we'd remove the node_modules folder in the fixture folder, would we still get the same test results? This is usually necessary to have in certain situations with binaries and such. If it's not doing much we might as well delete it.

@@ -1,5 +1,5 @@
export type BaseStyleLintConfig = {
customSyntax?: string;
customSyntax?: string | object;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe unknown would be better than object here?

@TJNhxMZHmqGytuWT
Copy link
Contributor Author

Thanks!

Idea: if we'd remove the node_modules folder in the fixture folder, would we still get the same test results? This is usually necessary to have in certain situations with binaries and such. If it's not doing much we might as well delete it.

If we completely remove node_modules, we get some errors because some of these packages are directly imported with require() in Stylelint config files

But I removed the ones that weren't necessary!

@webpro webpro merged commit 41a2bcc into webpro-nl:main Feb 12, 2025
@webpro
Copy link
Collaborator

webpro commented Feb 12, 2025

Perfect, thank you!

@webpro
Copy link
Collaborator

webpro commented Feb 12, 2025

🚀 This pull request is included in v5.44.1. See Release 5.44.1 for release notes.

Using Knip in a commercial project? Please consider becoming a sponsor.

@TJNhxMZHmqGytuWT TJNhxMZHmqGytuWT deleted the knip-stylelint-config-js branch February 12, 2025 23:07
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.

2 participants