thheller / shadow-cljs

ClojureScript compilation made easy
https://github.com/thheller/shadow-cljs
Eclipse Public License 1.0
2.24k stars 175 forks source link

Rework :npm-deps handling & support #998

Open p-himik opened 2 years ago

p-himik commented 2 years ago

Clojurians Slack discussion thread: https://clojurians.slack.com/archives/C6N245JGG/p1647112910767939

Options:

According to what I gather from npm docs, there seems to be yet another option. You can concatenate versions with spaces, it won't break anything since there's only one binary operation, ||, and it has the highest precedence - the spaces are implied as &&. So something like >1 <8 >2 >3 || <1 is a valid version string. So:

@thheller has mentioned that "forking a node process has other issues". I don't know what those issues are, but npm does use node, and shadow-cljs does shell out to npm when installing dependencies.

thheller commented 2 years ago

To put this into more context that doesn't have to be reconstructed from slack history:

shadow-cljs currently uses a bundled semver.js file via the graal-js ScriptEngine to eliminate possible conflicts in :npm-deps from deps.cljs files. I can't be bothered to figure out how all the version range stuff works in npm, so I opted to use the default node package implementation for this instead of reimplementing it in Clojure.

The problem this is intended to solve is that some CLJS libraries include a deps.cljs file with :npm-deps to declare their dependency on some npm packages. Not all but some use version ranges here. So you can end up in situations where a CLJS lib wants npm dep ^17.0.5 vs another wanting ^16.5.6 or whatever else range pattern that may be used. I haven't done a full audit on all CLJS packages, so I cannot say how common more complex ranges are. However this conflict needs to be resolved so shadow-cljs can trigger the actual install of said packages. Only one declaration can be installed. This is where the semver.js file comes into play in deciding which one to pick. If the ranges intersect it can automatically pick one, if they don't it currently just warns.

This used to use the nashorn JS engine which comes bundled with the JDK pre v15 but was removed in that release. So as a quick fix I decided to use the "replacement" graal-js engine instead. This however nowadays causes warnings when the user uses the actual full GraalVM. It is also a rather large dependency that most people will never use, given that only a deps.cljs conflict will trigger it.

So the goal should be to get rid of graal-js entirely. So some replacement for semver.js is needed. Either running in node or implemented in java/clj.

I'm also sort of considering dropping the automated install entirely. While convenient it is not perfect either.

It might also be OK to drop the version ranges completely to make comparison easier. Regular versions are easy to compare, so we could just pick the higher version of a given range. ^16.7.0 over ^16.0.0 is trivial. Should do an audit how wild people get in deps.cljs files.

thheller commented 2 years ago

In 2.18.0 I have removed support for the semver checks entirely and just use the first deps.cljs :npm-deps version specified for each package. This is the simplest solution that allows me to get rid of the graal-js engine.

I might consider reworking this again at some point but for now this should be good enough.

If anyone is seriously interested in working on this you could totally do this as a completely separate library. Nothing in this is dependent on shadow-cljs. As a separate lib/tool this could also then be used together with figwheel or just cljs.main. It is just a separate command that needs to get the deps.cljs files from the classpath (as done here, feel free to copy) and then install the packages somehow. You could use the entire npm_deps.clj namespace as a reference.

p-himik commented 2 years ago

There's a slight leftover: https://github.com/thheller/shadow-cljs/blob/c2d8e13a562f3a774720fb130f172e42896eb8ca/src/main/shadow/cljs/npm/check_versions.clj#L42

No clue whether it affects anything - I just noticed it by accident.

thheller commented 2 years ago

Good catch. That used to be part of the :browser build process checking the installed version but ended up more noise than actual usable info. I removed it a while ago and made it a separate runnable -main. I don't think I ever told anyone about this so I'll just remove it entirely.

Build reports are also much better for understanding your builds anyways.