Skip to content

Write optionalDependencies explicitly in package{,-lock}.json#7672

Open
doronbehar wants to merge 2 commits intoYlianst:masterfrom
doronbehar:optionalDependencies
Open

Write optionalDependencies explicitly in package{,-lock}.json#7672
doronbehar wants to merge 2 commits intoYlianst:masterfrom
doronbehar:optionalDependencies

Conversation

@doronbehar
Copy link
Copy Markdown

Hello,

As part of the efforts to package this properly in Nixpkgs (continuation of #7643), we noticed the file ./meshcentral.js imports conditionally many dependencies. Iterating all modules.push lines in it, and running npm install "$p" --save-optional for each of these packages, modified package-lock.json as done here. The version of Meshcentral itself too had to be updated to 1.1.57 in package-lock.json, so it was done in a separate commit.

If you would be able to continue maintaining package-lock.json like this it would help us a lot.

Note

This package-lock.json file was generated with nodejs 22 - LTS version as stated in the docs.

@si458
Copy link
Copy Markdown
Collaborator

si458 commented Mar 9, 2026

can you explain this a little more?

the problem we have is meshcentral is designed to be run with as little packages as possible,
then you customise it and it installs packages that you need/want
i understand this is a bad idea in general but its how its been written all those years ago

also from my past experience using optionalDependencies is bad
because it still installs all those packages even tho you told it not too!
so our docker image for example would become huge!

also we DONT use ^ in the versions because package get updated too often and always break meshcentral
because npm installed a newer version of a package that we havent tested so it broke
so this is why we use static versions instead then update manually over time as new packages get updated!

@doronbehar doronbehar force-pushed the optionalDependencies branch from 8715a39 to c5ef3f5 Compare March 9, 2026 13:31
@doronbehar
Copy link
Copy Markdown
Author

doronbehar commented Mar 9, 2026

also we DONT use ^ in the versions because package get updated too often and always break meshcentral
because npm installed a newer version of a package that we havent tested so it broke
so this is why we use static versions instead then update manually over time as new packages get updated!

That can be solved easily by using npm install --save-exact, and was fixed in the force push done now.

also from my past experience using optionalDependencies is bad
because it still installs all those packages even tho you told it not too!
so our docker image for example would become huge!

I see, though that's easily fixed with --omit=optional, as done in the 3rd commit of the PR, pushed just now. Could you please see if this works for you?

@si458
Copy link
Copy Markdown
Collaborator

si458 commented Mar 9, 2026

@doronbehar unfortunately when i run npm install its still installing LOADS of extra packages that i do not need!
for example, i look after 100 devices, using the built in nedb which is fine,
why is it installing mariadb, mongodb or even passport (for oidc) !?

you are also installing extra packages like html-minifier-terser which is ONLY needed for translations,
not for general use at all?

also changing all our documentations etc to tell people to instead run npm install meshcentral --omit=optional
is NOT ideal!

i really dont think we can accept this PR at the moment, until a proper solution can be resolved/found

do you maybe need us to change the npm install command when we install extra packages on start up?

edit: just updated package-lock.json for you so it matches the package.json!
i keep forgetting to ask Ylian to update this whenever he does the month releases! #todolist

@doronbehar
Copy link
Copy Markdown
Author

@si458 I understand now. Would you be accepting a modification to package-lock.json file that will include these dependencies in devDependencies? We might be able to work something that will work for us with this.

@si458
Copy link
Copy Markdown
Collaborator

si458 commented Mar 9, 2026

@doronbehar ive never fully understood the package-lock.json! (being totally 110% honest here!)
from my understanding it tells npm these packages where used on this pc so you need to install these packages???
because in my eyes the package-lock.json should match whats inside the package.json ?
am i right or completely wrong?

@doronbehar
Copy link
Copy Markdown
Author

from my understanding it tells npm these packages where used on this pc so you need to install these packages???

Yes. The idea is reproducibility - users should use the exact same versions of dependencies used by the developer, to reduce the potential for mismatches. This is indeed not a hard requirement for most cases, but for Nix it is fully required.

@doronbehar
Copy link
Copy Markdown
Author

@si458 I now realized that using devDependencies would also require modifying the installation instructions to use env NODE_ENV=production npm install. I also think that optionalDependencies is more correct semantically. As for:

also changing all our documentations etc to tell people to instead run npm install meshcentral --omit=optional is NOT ideal!

It seems to me that you only write a npm install command in this installation guide, so there aren't many more places to correct this command line.

I asked a LLM to summarize the arguments for including these optionalDependencies:

Why optionalDependencies + --omit=optional is the right move:

  1. The documentation change is trivial. You've expressed concern about updating docs, but the install command appears only once. Changing npm install meshcentral to npm install meshcentral --omit=optional is a single-line diff with no architectural impact.
  2. Default behavior can stay the same. If you're worried about users forgetting the flag, you could even keep npm install meshcentral as the documented command - npm will then install optional deps by default, which will result in only a few tens of Megabytes difference.
  3. It uses npm exactly as designed. optionalDependencies exists precisely for dependencies that are conditionally needed at runtime. Your use case is the textbook example of what that field is for.
  4. It unblocks every downstream packager for free. NixOS, Debian, Homebrew — any distro that wants to ship MeshCentral properly currently can't, because the dependency graph is invisible until runtime. Tracking them in package-lock.json costs you nothing and fixes all of them simultaneously.
  5. It makes security tooling work. Right now runtime-installed modules are invisible to npm audit, Dependabot, and Renovate until after they're installed. Tracked dependencies get vulnerability scanning automatically.

@si458
Copy link
Copy Markdown
Collaborator

si458 commented Mar 10, 2026

@doronbehar I don't believe some of those points are valid.

For example we have a windows exe installer app which would need changing, rebuilding, signing etc.
docs would stil need changing,
everyone's memory would need changing (im not the M-I-B)

Also a few mb difference is a complete lie as its more like 100mb difference!!!

Also as explained u are installing packages that just arent required at runtime! Like the translate packages for example.
Or extra database engines.

Im not going to merge this until we have our community meeting at the end of the month, then the community can chat about it and we can hear their opinions

Please do come join the meeting tho so we can talk about, and i don't forget!

Was done using the following (with nodejs_22):

```
npm install \
	passport@0.7.0 \
	connect-flash@0.1.1 \
	passport-twitter@1.0.4 \
	passport-google-oauth20@2.0.0 \
	passport-github2@0.1.12 \
	passport-azure-oauth2@0.1.0 \
	jwt-simple@0.5.6 \
	openid-client@5.7.1 \
	passport-saml \
	@duosecurity/duo_universal@2.1.0 \
	node-windows@0.1.14 \
	loadavg-windows@1.1.1 \
	node-sspi@0.2.10 \
	ldapauth-fork@5.0.5 \
	ssh2@1.17.0 \
	svg-captcha@1.4.0 \
	image-size@2.0.2 \
	acme-client@4.2.5 \
	aedes@0.51.3 \
	mysql2@3.15.1 \
	@mysql/xdevapi@8.0.33 \
	mongodb@4.17.2 \
	@mongodb-js/saslprep@1.3.1 \
	pg@8.16.3 \
	mariadb@3.4.5 \
	acebase@1.29.5 \
	sqlite3@5.1.7 \
	node-vault@0.10.5 \
	https-proxy-agent@7.0.6 \
	mongojs@3.1.0 \
	nodemailer@6.10.1 \
	@sendgrid/mail@8.1.6 \
	jsdom@22.1.0 \
	esprima@4.0.1 \
	html-minifier-terser@7.2.0 \
	@crowdsec/express-bouncer@0.1.0 \
	prom-client@15.1.3 \
	archiver-zip-encrypted@2.0.0 \
	googleapis@128.0.0 \
	webdav@5.9.0 \
	minio@8.0.6 \
	wildleek@2.0.0 \
	yub@0.11.1 \
	image-size@2.0.2 \
	twilio@4.23.0 \
	plivo@4.75.1 \
	telnyx@1.25.5 \
	telegram@2.26.22 \
	input@1.0.1 \
	discord.js@14.6.0 \
	@xmpp/client@0.13.6 \
	node-pushover@1.0.0 \
	zulip@0.1.0 \
	web-push@3.6.7 \
	firebase-admin@12.7.0 \
	syslog@0.1.1-1 \
	--save-optional --save-exact
```

Then, to work around [this bug][1], I ran:

```
npm install
```

[1]: npm/cli#7530
@doronbehar doronbehar force-pushed the optionalDependencies branch from c5ef3f5 to def280e Compare March 10, 2026 09:31
@doronbehar
Copy link
Copy Markdown
Author

OK I'm too would be in favor of using

Also as explained u are installing packages that just arent required at runtime! Like the translate packages for example.

I didn't add directly any translation packages. The commit message of the current PR commit documents exactly what I ran.

Also a few mb difference is a complete lie as its more like 100mb difference!!!

I see. Still though, adding --omit=optional shouldn't be too hard for all places where it appears (2 Dockerfiles modified in this PR and the installation guide).

Im not going to merge this until we have our community meeting at the end of the month, then the community can chat about it and we can hear their opinions

Please do come join the meeting tho so we can talk about, and i don't forget!

Sure I'd like to express this concern. Please tell me where will I be able to get a link to the meeting etc.

@si458
Copy link
Copy Markdown
Collaborator

si458 commented Mar 10, 2026

The next meeting is 26th March 2026 at 2pm UTC (PLZ CONVERT TO YOUR TIMEZONE)
https://github.com/Ylianst/MeshCentral/wiki/Community-Monthly-Meetings

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.

2 participants