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: add package version consistency check #515

Merged
merged 3 commits into from
Sep 22, 2021

Conversation

erossignon
Copy link
Contributor

Hi, I would like to propose this utility that will help to detect misalignment of packages version.

This tool scans the subpackage folder and analyzes their dependencies/devDependencies.
It searches for packages that appear in multiple modules but have different versions and gives you hints to fix the package.json files.

Finally, it gives you a list of all packages used by all modules of the mono repo that we can add to the devDependencies section of the main package.json.
By doing this we allow all shared modules to be installed once at the top level.

By using it, we can reduce the installed size from 2.1Gb to 500mb

image

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.

First of all thank you for your contribution, reducing the total installation size is great 🎉 . I have a couple of concerns that need to be addressed:

  1. the minimum node required version for node-wot is 12, but all node types are updated to 16.x. It should really be 12.x
  2. I am not sure that adding all the dev dependencies indiscriminately to the root package module is providing value. Some shared dependencies may be indeed added there, like tsc or ts-node, but I am seeing also vm2, coap, mqtt, why are there?

By the way, I had the impression that npm workspaces should have already handled packages with the same version and install them once in the root module. isn't that so? why do we need to add "everything" as dev dependency in the root package.json ?

@danielpeintner
Copy link
Member

danielpeintner commented Sep 20, 2021

Note: I think the idea of having all dependencies at the root level is the following: Doing so prohibits downloading the same dependency in subfolders.

Anyhow, I guess we need to check whether our change from lerna to npm workspaces solved the issue already.

@relu91
Copy link
Member

relu91 commented Sep 20, 2021

So I did some local tests with a toy project. Project structure:

+-- package.json
`--  a
   `-- package.json
`--  b
   `-- package.json

If I install the same typescript version on the two workspaces I get the following:

+-- node_modules
|  `-- a -> ../a
|  `-- b -> ../b
|  +-- typescript
|  +-- .bin
+-- package-lock.json
+-- package.json
`-- a
   `-- package.json
`--  b
   `-- package.json

If package b has a different typescript version, I get the following:

+-- node_modules
|  `-- a -> ../a
|  `-- b -> ../b
|  +-- typescript
|  +-- .bin
+-- package-lock.json
+-- package.json
`-- a
   `-- package.json
+ --  b
| +-- node_modules
| |  `-- a -> ../a
| |  `-- b -> ../b
| |  +-- typescript
| |  +-- .bin
|  `-- package.json

In both cases, the root package JSON was not touched. My conclusion is that we need a version alignment for every dev tool inside node-wot but this would not imply providing every dependency in the root package.

What I found online is that it might be a good practice to include typescript, and other build tools in the root package.json but this does not really influence the final node_modules size (you can still have different tool versions on sub-packages).

@danielpeintner
Copy link
Member

danielpeintner commented Sep 20, 2021

Some more comments (summary):

@erossignon
Copy link
Contributor Author

erossignon commented Sep 20, 2021

What I found online is that it might be a good practice to include typescript, and other build tools in the root package.json but this does not really influence the final node_modules size (you can still have different tool versions on sub-packages).

You are correct ! there is no need to duplicate modules in the main package.json, just having version consistency is enough.
I have updated the PR, by renaming the script file, moving it to utils, and refactoriung it slightly.

the script is now called utils/check_package_version_consistency.js

@erossignon erossignon requested a review from relu91 September 20, 2021 16:21
@erossignon
Copy link
Contributor Author

erossignon commented Sep 20, 2021

I have also added a dedicate step in the build system that enforces version consistency verification.

npm run check:versions

@erossignon
Copy link
Contributor Author

the minimum node required version for node-wot is 12, but all node types are updated to 16.x. It should really be 12.x

It doesn't harm to use @types/node@16 for typescript definition as it is only for type checking. Btw using @types/node@16 allowed me to find that some stream encoding was mispelled => uf8 instead of utf8.

At the end, node12 is used in the CI/CD. this is what counts. Some users might want to run node-wot with node16.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@danielpeintner
Copy link
Member

danielpeintner commented Sep 21, 2021

@erossignon I see Eclipse / ECA issue?
Did you sign that the ECA ?

You have to associate your account with it, see also https://github.com/apps/eclipse-eca-validation

package.json Outdated Show resolved Hide resolved
@JKRhb
Copy link
Member

JKRhb commented Sep 21, 2021

@erossignon I see Eclipse / ECA issue?
Did you sign that the ECA ?

You have to associate your account with it, see also https://github.com/apps/eclipse-eca-validation

I am afraid the issue is caused by 809ac9b which was the result of a suggestion by me. @erossignon Maybe you can squash the commit with a CI related commit from before to avoid the issue?

@erossignon erossignon force-pushed the FixPackageVersion branch 2 times, most recently from e8836f6 to 90ef6e9 Compare September 21, 2021 18:28
@erossignon
Copy link
Contributor Author

@erossignon I see Eclipse / ECA issue?
Did you sign that the ECA ?
You have to associate your account with it, see also https://github.com/apps/eclipse-eca-validation

I am afraid the issue is caused by 809ac9b which was the result of a suggestion by me. @erossignon Maybe you can squash the commit with a CI related commit from before to avoid the issue?

Thank ! I squashed it.

@erossignon erossignon changed the title Fix package version chore: add package version consistency check Sep 21, 2021
@erossignon erossignon force-pushed the FixPackageVersion branch 2 times, most recently from def4ddc to 64d6108 Compare September 21, 2021 19:47
@danielpeintner
Copy link
Member

Thanks @erossignon

the latest changes

  • created a merge conflict with .github/workflows/ci.yaml
  • re-introduced the changes about devDependencies in root package.json
  • I think you also need to call npm run format to get the coding style right since i see some double spaces et cetera (ie. ci.yaml)

@erossignon
Copy link
Contributor Author

  • re-introduced the changes about devDependencies in root package.json
    This s on purpose;
    I suggest that we install typescirpt and eslint and mocha tooling at the top level

@erossignon
Copy link
Contributor Author

npm run format has been run

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 great now! I have still a concern about the node version, to me is quite annoying to build an entirely new feature (maybe using a new nodejs API) and realizing only in the CI that I can't use it. Build time checks are important and make development faster. However, this is my workflow style and if others are ok to bump up node type I am fine either way 👍🏻

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

IF the node version 16 causes issues we can revert this change

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