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(noUndeclaredDependencies): add availability restriction based on dependency type #4376

Merged

Conversation

siketyan
Copy link
Contributor

Summary

This pull request adds dependency availability restriction based on their type and the file path.

For example, we don't want to allow imports from vitest outside the test suites:

{
  "options": {
    "devDependencies": ["**/*.test.ts"]
  }
}

When this option is set, dependencies in devDependencies will be considered it's available only in **/*.test.ts files.

Inspired from import/no-extraneous-dependencies.

Test Plan

See added test snapshots.

… dependency type

Signed-off-by: Naoki Ikeguchi <me@s6n.jp>
@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Oct 24, 2024
@siketyan siketyan changed the title feat(noUndeclaredDependencies): Add availability restriction based on dependency type feat(noUndeclaredDependencies): add availability restriction based on dependency type Oct 24, 2024
Copy link

codspeed-hq bot commented Oct 24, 2024

CodSpeed Performance Report

Merging #4376 will not alter performance

Comparing siketyan:feat/noUndeclaredDependencies_devDependencies (2673379) with main (3401663)

Summary

✅ 99 untouched benchmarks

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Nice work!

@siketyan
Copy link
Contributor Author

@arendjr @Conaclos Thank you for your review! I've added commits to resolve the suggestions.

@siketyan siketyan force-pushed the feat/noUndeclaredDependencies_devDependencies branch from fc2424a to 5c3bdc0 Compare October 25, 2024 14:08
@siketyan
Copy link
Contributor Author

@ematipico could you please take a look this (and merge if no problem) when you have time? No hurries, just a friendly reminder :)

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Awesome work! ❤️ I left some suggestions for the docs, then we can merge it

Comment on lines 62 to 73
/// You can also use an array of globs instead of literal booleans:
/// ```json
/// {
/// "options": {
/// "devDependencies": ["**/*.test.js", "**/*.spec.js"]
/// }
/// }
/// ```
///
/// When using an array of globs, the setting will be set to `true` (no errors reported)
/// if the name of the file being linted (i.e. not the imported file/module) matches a single glob
/// in the array, and `false` otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// You can also use an array of globs instead of literal booleans:
/// ```json
/// {
/// "options": {
/// "devDependencies": ["**/*.test.js", "**/*.spec.js"]
/// }
/// }
/// ```
///
/// When using an array of globs, the setting will be set to `true` (no errors reported)
/// if the name of the file being linted (i.e. not the imported file/module) matches a single glob
/// in the array, and `false` otherwise.
/// You can also use an array of globs instead of literal booleans.
/// When using an array of globs, the setting will be set to `true` (no errors reported)
/// if the name of the file being checked (i.e. not the imported file/module) matches a single glob
/// in the array, and `false` otherwise.
///
/// TODO: explain the example here
///
/// ```json
/// {
/// "options": {
/// "devDependencies": ["tests/*.test.js", "tests/*.spec.js"]
/// }
/// }
/// ```
///

Copy link
Member

Choose a reason for hiding this comment

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

I left a TODO at line 67. Usually, in our docs, we follow this pattern:

  • explain the options
  • explain the example
  • show code of the example

The TODO should explain the example of the code. Also, I removed **/ because it usually matches too much and replaced it with a more simple glob

siketyan and others added 2 commits October 31, 2024 01:43
…suggestion

Co-authored-by: ematipico <my.burning@gmail.com>
Signed-off-by: Naoki Ikeguchi <me@s6n.jp>
# Conflicts:
#	packages/@biomejs/backend-jsonrpc/src/workspace.ts
@siketyan
Copy link
Contributor Author

@ematipico Thank you for your suggestion! Applied in ba4e63d. (Also merged main resolving conflicts)

@ematipico ematipico merged commit 3401663 into biomejs:main Oct 30, 2024
3 checks passed
@siketyan siketyan deleted the feat/noUndeclaredDependencies_devDependencies branch November 3, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants