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

[New] no-extraneous-dependencies: Add considerInParents option #2481

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

luxaritas
Copy link

Resolves #1913

I am not particularly happy with the implementation (in particular, the error passing and "requireExact" handling to match the current behavior feel misplaced and/or mishandled), but it's at the point that I feel it would be useful to get feedback (both on how the options are handled and the implementation).

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Base: 95.27% // Head: 95.23% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (a686917) compared to base (c3d14cb).
Patch coverage: 98.07% of modified lines in pull request are covered.

❗ Current head a686917 differs from pull request most recent head 61eddb2. Consider uploading reports for the commit 61eddb2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2481      +/-   ##
==========================================
- Coverage   95.27%   95.23%   -0.05%     
==========================================
  Files          68       66       -2     
  Lines        2944     2793     -151     
  Branches      998      940      -58     
==========================================
- Hits         2805     2660     -145     
+ Misses        139      133       -6     
Impacted Files Coverage Δ
utils/readPkgUp.js 96.42% <95.23%> (+14.61%) ⬆️
src/rules/no-extraneous-dependencies.js 100.00% <100.00%> (+1.78%) ⬆️
utils/parse.js 98.14% <0.00%> (-0.13%) ⬇️
src/rules/dynamic-import-chunkname.js 96.77% <0.00%> (-0.11%) ⬇️
src/rules/order.js 99.10% <0.00%> (-0.09%) ⬇️
src/rules/no-cycle.js 97.91% <0.00%> (-0.05%) ⬇️
src/rules/export.js 100.00% <0.00%> (ø)
src/rules/no-restricted-paths.js 100.00% <0.00%> (ø)
src/rules/no-anonymous-default-export.js 100.00% <0.00%> (ø)
src/rules/consistent-type-specifier-style.js
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

docs/rules/no-extraneous-dependencies.md Outdated Show resolved Hide resolved
src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
utils/readPkgUp.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title [New] no-extranious-dependencies: Support package.json files in parent folders [New] no-extraneous-dependencies: Support package.json files in parent folders Aug 29, 2022
@ljharb ljharb marked this pull request as draft August 29, 2022 19:05
@luxaritas
Copy link
Author

FYI, I have seen these comments (thanks! good points). Hopefully I'll have time within the next week or two to address them.

@luxaritas luxaritas force-pushed the feat/nested-package-json branch 2 times, most recently from c65ed54 to 61eddb2 Compare October 20, 2022 20:01
@luxaritas luxaritas changed the title [New] no-extraneous-dependencies: Support package.json files in parent folders [New] no-extraneous-dependencies: Add considerInParents option Oct 20, 2022
@luxaritas luxaritas marked this pull request as ready for review October 20, 2022 21:44
@luxaritas
Copy link
Author

Sorry for the delay! Rewrote this with your feedback and it is significantly simpler. :) As a bonus, this should also improve caching, since it's no longer necessary to touch the file system to look for the closest package.json. Let me know if there's anything further I can improve!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Looks great!

docs/rules/no-extraneous-dependencies.md Outdated Show resolved Hide resolved
docs/rules/no-extraneous-dependencies.md Outdated Show resolved Hide resolved
src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
@luxaritas
Copy link
Author

Resolved all the feedback - let me know if you want me to squash.

@WangHoHan
Copy link

What is the status of this PR?

@luxaritas
Copy link
Author

Waiting for rereview as far as I'm concerned

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This looks great! Sorry for the long review delay.

My comments are about converting the internal considerInParents to a Set for perf. It'd also be nice to add more tests such that all of the dependency types are covered.

src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
src/rules/no-extraneous-dependencies.js Outdated Show resolved Hide resolved
@ljharb ljharb marked this pull request as draft January 11, 2023 00:33
@luxaritas
Copy link
Author

Interesting idea to use set. I'd be interested to see that benchmarked given the small sizes and additional spread when creating the cache key, though I wouldn't immediately know how to do that myself. :) I can certainly add a few more tests.

@ljharb
Copy link
Member

ljharb commented Jan 11, 2023

yeah lol for such a small array it probably doesn't matter, but might as well use the proper data structure :-p

@luxaritas
Copy link
Author

@ljharb Can you help me understand why two of the tests I added are failing? It looks like this rule is trying to resolve any found import, even if it isn't present, and returns successfully if it isn't found? That.. can't be right, can it?

@ljharb
Copy link
Member

ljharb commented Apr 13, 2023

Hmm, no, I don't know why it's failing :-/ i just rebased it so results are fresh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

no-extraneous-dependencies doesn't support nested package.json
3 participants