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

detect phantom dependencies and fix #1321

Conversation

hasanheroglu
Copy link
Contributor

Closes #1319.

As @relu91 mentioned, separating CI runs gets complicated and doesn't match the logic of a monorepo. I found a better and simpler solution: adding import/no-extraneous-dependencies to eslint configuration.

This lint config deals with "phantom dependencies" and gives an error if they exist.

@JKRhb
Copy link
Member

JKRhb commented Aug 22, 2024

Thank you, @hasanheroglu, that looks very promising :) I am wondering, though: Does this PR make the comon devDependencies in the top-level package.json obsolete? If so, then that section could probably be removed.

Consdering the conflicts, you may need to rebase your branch against master to resolve them.

@hasanheroglu
Copy link
Contributor Author

Thank you, @hasanheroglu, that looks very promising :) I am wondering, though: Does this PR make the comon devDependencies in the top-level package.json obsolete? If so, then that section could probably be removed.

I just realized I didn't add devDependencies for types in the packages. I can add them and then remove the obsolete dependencies from the root.

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.46%. Comparing base (36b3f79) to head (91ada19).
Report is 27 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1321      +/-   ##
==========================================
+ Coverage   76.26%   77.46%   +1.19%     
==========================================
  Files          82       83       +1     
  Lines       15946    16576     +630     
  Branches     1603     1667      +64     
==========================================
+ Hits        12162    12841     +679     
+ Misses       3731     3678      -53     
- Partials       53       57       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@relu91
Copy link
Member

relu91 commented Aug 22, 2024

Great, that's an ingenuous move 👍🏻 well done. The only problem that I see is that having dev deps in the root package.json help with the consistency of the dev tools among all packages... however, I always feel that is not a big deal since every package could in theory be developed differently than the others (with different dev tools). @danielpeintner what do you think?

@hasanheroglu
Copy link
Contributor Author

hasanheroglu commented Aug 22, 2024

Great, that's an ingenuous move 👍🏻 well done. The only problem that I see is that having dev deps in the root package.json help with the consistency of the dev tools among all packages... however, I always feel that is not a big deal since every package could in theory be developed differently than the others (with different dev tools). @danielpeintner what do you think?

Actually, we can configure the lint rule to apply only to product dependencies. Therefore, we don't need to change any devDependencies, and it shouldn't cause any issues for the package users.

@danielpeintner
Copy link
Member

however, I always feel that is not a big deal since every package could in theory be developed differently than the others (with different dev tools). @danielpeintner what do you think?

I still think it would be better to keep the versions consistent. Apart from the package.json in root I think the following script should check the consistency also, right?

"check:versions": "node utils/check_package_version_consistency.js",

We do run it also on the CI workflow.

version_consistency:

This makes me think that we do not really need package.json in the root, right?

@relu91
Copy link
Member

relu91 commented Aug 22, 2024

This makes me think that we do not really need package.json in the root, right?

It is still required for generic monorepo configuration and lifecycle scripts, but where to list dev dependencies is completely up to us.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Anyhow I forgot to approve the PR.

@hasanheroglu
Copy link
Contributor Author

So I added eslint configs, that extend the root config override "no-extraneous-dependencies" rule, to test directories. Therefore, the rule only applies to the product dependencies and we can share common devDependencies in the root package.json.

Copy link
Member

@JKRhb JKRhb left a comment

Choose a reason for hiding this comment

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

Thank you, @hasanheroglu!

@relu91 relu91 merged commit daf620a into eclipse-thingweb:master Aug 28, 2024
13 checks passed
@relu91
Copy link
Member

relu91 commented Aug 28, 2024

thanks!!!

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.

Detect issue with dependencies in monorepo
4 participants