Request: reconsider the package resolution policy

[kyptin]

In issue #547, you make it clear that you do not intend to mimic the way Node.js resolves dependencies. I'd like to ask you to reconsider that.

Let me first say that I respect your right as the creator of Shadow CLJS to do whatever the heck you want with it. I'm grateful you've made a tool that so many have found useful, and if you decline this request, I will understand.

But what I thought I'd add to the conversation is a small working example demonstrating the problems that Shadow's dependency resolution policy has caused for a colleague (@christopherlaangsell) and me on a project, as well as a discussion of the (perhaps unappreciated) confusion and difficulty it causes.

The full example, complete with thorough documentation, is here:
https://github.com/kyptin/shadow-cljs-example-breakage

As a summary, the problem is with a single dependency, evergreen-ui, which depends on two other dependencies, glamor and ui-box, which depend on different, incompatible versions of a fourth dependency, inline-style-prefixer (abbreviated ISP below). It's as simple an example as I can imagine that captures the issue.

Our difficulties started when certain components in Evergreen didn't seem to work as advertised. We'd get perplexing errors that didn't seem to relate to our code. We eventually gave up trying to diagnose the issue within the project and created a minimal example that triggered the same error. We created another minimal example—this one in JS—that had equivalent code but did not have the error.

We eventually discovered that the version of the ISP dependency actually included in the CLJS page was not what glamor needed. Once we investigated further, we learned that the ISP dependency had a major breaking change at v5.0.0, such that packages expecting pre-v5 versions were virtually guaranteed to be incompatible with post-v5 versions, and vice versa.

We've had other mysterious errors in the past. In most cases, after sinking some time in, we punt and give up on using some library. Looking back, we wonder whether those were instances of the same constraint imposed by Shadow. The point is that it's difficult to determine the effects of this Shadow constraint.

Looking forward, we expect to hit other mysterious errors, at which point, we're not eager to dig into package-lock.json files and changelogs of dependencies (assuming they exist) to diagnose the issue. With respect, we're likely to abandon Shadow CLJS in favor of Figwheel, which, to the best of our knowledge, does not have this constraint and so is immune from these issues.

Let me also say that I appreciate that you are focusing (e.g. in #547) on what will be safer for users of Shadow CLJS, and trying to help beginners avoid shooting themselves in the foot. However, this issue turned out to be a significant stumbling block for us, even with this simple example project. We expect that it would actually be less of a footgun to mimic Node.js in this respect.

You might say that Evergreen should be more consistent in its dependencies and ensure that it never needs multiple versions of a dependency. However, I can't think of an argument to persuade Evergreen to do this. No Node.js user feels any pain from Evergreen needing multiple versions of ISP. Only users of Shadow CLJS feel the pain here. So these kinds of problems are not likely to go away. No pain is driving their solution.

Furthermore, having multiple versions of packages is an incredibly common thing in Node.js projects. For example, even in our small example, we had 25 instances of version overrides (as counted by this ugly command: jq '.dependencies | .[] | .dependencies | length' package-lock.json | perl -ne '$sum+=$_; END{print "$sum\n"}'). (In our larger project from which the example derived, we had 464 instances.) The reason Shadow's constraint is not more noticeable is because few of these version overrides include breaking changes—or at least as profoundly broken as ISP at v5. But any of these overrides could include changes with subtle or severe breaks.

So, practically speaking, I think it's important either to conform Shadow CLJS to Node.js's dependency resolution policy (which is actually not complex and is described here), or else to describe this limitation more prominently and more completely in the Shadow CLJS documentation.

Thanks for considering this. And for Shadow, which, despite this limitation, has still brought us a lot of value.

[thheller]

I do agree that this is a problem.

As an immediate fix you could just use webpack as described here. Since webpack (or any other tool you chose) would then be resolving the JS dependencies you'd also get whatever resolve rules they use. At the expense of having to setup that tool, but that doesn't seem to be a blocker for you as you'd need to do the same with figwheel.

As for addressing the problem one thing that definitely needs to be fixed is that this goes by mostly unnoticed. There should be a warning of some kind if a duplicated package is ignored.

I might be open to adding a config option to then allow using that duplicated package. I will however not make this the default behavior. I think this is terrible and the fact that it is so common shows precisely why this needs to be fixed somehow. I have seen blog posts from people optimizing their builds and cutting several megabytes of redundant code just by fixing duplicated packages.

I'm aware that optimizing for build-size here isn't exactly the most developer-friendly choice but I'm a firm believer that it is the most user-friendly. Dealing with dependency conflicts is never fun. Sweeping them under the rug and ignoring them however is not an acceptable way to handle this in my view.

[kyptin]

Thanks for the quick (and sympathetic) reply.

Good to know about the possible workaround. We'll look into that.

Agreed that having some warning would be helpful. However, I'll note that this might be a vexing source of noise for those who don't run into these problems, given how pervasive the problem is in the NPM ecosystem.

Adding an opt-in option for Node-style dependency resolution sounds like a good idea to me. That would definitely address our need.

Lastly, I appreciate your push for dependency hygiene, and I hope others heed that call. I wonder how avoidable the problem is, but it does seem bad in NPM-land. (Maybe it's this bad everywhere and I just haven't looked closely; I don't know.)

[thheller]

FWIW there is already one warning for this:

npm package "ui-box" expected version "inline-style-prefixer@^5.0.4" but "3.0.8" is installed.

It however only shows up when using --verbose, eg. shadow-cljs watch app --verbose.

As you already pointed out however those warnings are often pointless and generate unnecessary noise, so bumping the log level so it always shows up might have many negative effects given how common this is.

[kyptin]

To follow up, we tried the webpack approach to work around this problem, and it worked well. Thanks for suggesting that. I guess we just need to remember to re-run webpack after npm install (or any other command that affects the contents of node_modules/).

I still think having an opt-in setting to use NPM-style dependency resolution could be helpful. Or, failing that, at least a mention in the documentation of how the simpler Shadow-style resolution can produce surprising bugs with misleading error messages. But now that we've understood and worked around the issue in our project, we are past the point of feeling this pain ourselves. (Thanks again.)

[thheller]

I really couldn't think of a proper way to deal with this so duplicated packages are now allowed as of 2.15.0. Build reports now also show duplicates. Hope that it doesn't end up too common for most people.

[kyptin]

Wow, thanks very much! I share your hope that most projects can avoid this kind of complexity—and I appreciate your concession to pragmatism. Thanks again. ❤️