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

Rework :npm-deps handling & support #998

Open
p-himik opened this issue Mar 13, 2022 · 4 comments
Open

Rework :npm-deps handling & support #998

p-himik opened this issue Mar 13, 2022 · 4 comments

Comments

@p-himik
Copy link
Sponsor

p-himik commented Mar 13, 2022

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

Options:

  • Keep parsing and comparing ranges via one of the Java SemVer libraries
  • Just install the highest version and hope for the best
  • Install something according to one found version range and warn if there are other version ranges that aren't byte-for-byte the same
  • Like the above but make it an interactive command

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:

  • Gather all :npm-deps
  • merge-with them with concatenating the version strings with spaces (replace empty versions with "*")
  • Add to those the contents of package.json, but only for dependencies that were in :npm-deps
  • Run npm install for every accumulated dependency with an explicit version
  • If there are conflicts, npm will loudly fail. If there are no conflicts, the required dependencies will be installed, if they haven't been installed before.

@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 thheller changed the title Consider getting rid of or replacing semver.js Rework :npm-deps handling & support Mar 15, 2022
@thheller
Copy link
Owner

thheller commented Mar 15, 2022

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 cannot resolve conflicts with already installed packages, and as such will not trigger installs of packages that are already in package.json.
  • It currently only tries to install in the project root package.json. Any build using :js-package-dirs may not end up using them at all, so no point in installing them in the first place.
  • Regular cljs.main has a install-deps command. shadow-cljs should too probably, instead of running this automatically every time on startup. Such a command could interactively ask which version to install.

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
Copy link
Owner

thheller commented Apr 4, 2022

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
Copy link
Sponsor Author

p-himik commented Apr 22, 2022

There's a slight leftover:

:when (not (npm-deps/semver-intersects wanted-version installed-version))]

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

@thheller
Copy link
Owner

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.

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

No branches or pull requests

2 participants