Skip to content

Fixes for Mocha plugin#1089

Merged
webpro merged 2 commits intowebpro-nl:mainfrom
apeloquin-agilysys:mocha-fix
May 20, 2025
Merged

Fixes for Mocha plugin#1089
webpro merged 2 commits intowebpro-nl:mainfrom
apeloquin-agilysys:mocha-fix

Conversation

@apeloquin-agilysys
Copy link
Copy Markdown
Contributor

The mocha plugin has the following issues:

  • Relies on presence of an explicit require or import of mocha in source code, but Mocha does not.
  • Does not recognize dependencies (e.g. ts-node/register or dotenv/config) in the requires configuration field.

The mocha test has the following issues:

  • isEnabled returns false since the fixture package.json dependencies do not include mocha; so it does not actually test the plugin
  • fixture config has extension , which plugin does not support, and does not have spec which the plugin does support

This PR addresses:

  • isEnabled will return true if default Mocha config file is present.
  • resolveConfig adds mocha dependency
  • resolveConfig supports both entries and dependencies from the require field
  • test fixture is updated to:
    • use spec instead of extension
    • include the mocha, @types/mocha, and ts-node dependencies in package.json
    • include both file and dependency values for require

Copy link
Copy Markdown
Member

@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, Aaron! Much appreciated. Just not sure I agree with the isEnabled changes. Any chance you could take a look?

Comment thread packages/knip/src/plugins/mocha/index.ts Outdated
Comment thread packages/knip/src/plugins/mocha/index.ts Outdated
Comment thread packages/knip/src/plugins/mocha/index.ts Outdated
@@ -1,4 +1,4 @@
{
"extension": ["ts"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is https://mochajs.org/#-extension-ext or is this only supported as command-line argument?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed it from the test because the plugin doesn't support it and spec with a glob overrules it. Figured the test should be matched to the plugin implementation.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented May 17, 2025

Open in StackBlitz

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

commit: 389c4fb

@webpro webpro merged commit 3424a5b into webpro-nl:main May 20, 2025
29 checks passed
@webpro
Copy link
Copy Markdown
Member

webpro commented May 20, 2025

Thanks, Aaron! Much appreciated. Good discussion to better understand how Knip's being used, feel free to open other issues/RFCs/PRs as you see fit.

@webpro
Copy link
Copy Markdown
Member

webpro commented May 20, 2025

🚀 This pull request is included in v5.57.0. See Release 5.57.0 for release notes.

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

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