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

chore: move common devDependencies to top-level package.json #1098

Merged
merged 6 commits into from
Sep 26, 2023

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Sep 22, 2023

Out of curiosity, I tried out if you can reduce redundancy in the package.json files by including shared devDependencies only in the top-level package.json file – and apparently, that works :)

This might require some double-checking but it could simplify updating shared devDependencies like TypeScript, eslint, and prettier in the future.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (bd247a0) 75.52% compared to head (3c88f71) 75.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1098   +/-   ##
=======================================
  Coverage   75.52%   75.52%           
=======================================
  Files          80       80           
  Lines       16167    16167           
  Branches     1517     1517           
=======================================
  Hits        12210    12210           
  Misses       3918     3918           
  Partials       39       39           

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

@JKRhb JKRhb marked this pull request as ready for review September 22, 2023 17:30
@egekorkan
Copy link
Member

It looks ok for the mqtt package but in other ones I also see new packages introduced. Why is it the case?

@JKRhb
Copy link
Member Author

JKRhb commented Sep 24, 2023

It looks ok for the mqtt package but in other ones I also see new packages introduced. Why is it the case?

In cases where a devDependencies field continues to be present, some entries were only used by the package in question (the diff might be a bit misleading here). We could also move those to the top level, but I thought it might make more sense to keep such devDependencies limited to the package that actually uses them.

@JKRhb
Copy link
Member Author

JKRhb commented Sep 24, 2023

As some of the dependency versions got changed in the individual package.json files, I now reverted them back to the original versions to make it a bit clearer what has actually changed. A proper update should then be performed in a follow-up PR.

@relu91
Copy link
Member

relu91 commented Sep 25, 2023

I'm wondering, don't we have a script dedicated to this kind of management of the dev deps? I talking about this, if it failed to check consistency what is there for?

@JKRhb
Copy link
Member Author

JKRhb commented Sep 25, 2023

I'm wondering, don't we have a script dedicated to this kind of management of the dev deps? I talking about this, if it failed to check consistency what is there for?

Oh, that is a very good point! In theory, the script should have indicated this kind of redundancy before. However, I think the actual check is currently disabled due to this line:

I will investigate and update the script if possible :)

@JKRhb
Copy link
Member Author

JKRhb commented Sep 25, 2023

The script should now be fixed by 0452ce8

@relu91
Copy link
Member

relu91 commented Sep 25, 2023

Ok, thank you @JKRhb, my understanding is that in the future the script will pick up inconsistencies once again and if something is off the Github action will warn us. Is that correct?

@JKRhb
Copy link
Member Author

JKRhb commented Sep 25, 2023

Ok, thank you @JKRhb, my understanding is that in the future the script will pick up inconsistencies once again and if something is off the Github action will warn us. Is that correct?

Exactly :) If any devDependency could be moved to the top-level package.json, then the "Check version consistency of packages" workflow will fail.

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.

Looks good to me!! we should document somewhere the stack that we are using for writing tests so that newcomers don't accidentally add new testing frameworks like jest.

@JKRhb
Copy link
Member Author

JKRhb commented Sep 25, 2023

Looks good to me!! we should document somewhere the stack that we are using for writing tests so that newcomers don't accidentally add new testing frameworks like jest.

Very good point! Should we open an issue for that?

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

LGTM

@danielpeintner danielpeintner merged commit ab1e20a into eclipse-thingweb:master Sep 26, 2023
7 checks passed
@JKRhb JKRhb deleted the package-json branch September 26, 2023 09:10
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.

4 participants