Page MenuHomePhabricator

[chronik-client] Install ecashaddrjs from npm for published version
ClosedPublic

Authored by bytesofman on Sep 25 2024, 22:42.

Details

Summary

The installation of monorepo-managed dependencies directly from the monorepo is best for CI testing and making sure that all monorepo apps and dependencies are always using (and working with) the latest version of monorepo-maintained dependencies.

However, a typical user installing a monorepo-managed dependency from npm will not get these relative installs and will need to install them manually as peer dependencies. This is not documented and we have at least one report of it not working anyway.

Test Plan

This builds locally. I followed these steps not in docker to publish an rc version, which a user reporting the issue said he was able to install and use successfully.

docker build -f chronik-client.Dockerfile -t chronik-client_local .

Event Timeline

Fabien requested changes to this revision.Sep 26 2024, 06:43
Fabien added a subscriber: Fabien.

This needs a rebase after D16812.

This diff solves a problem (chronik client install issues) and introduces a new one, as now the packages are no longer guaranteed in sync and ecashaddrjs can break all our apps silently.
We need to move to workspaces which I expect to solve both problems instead.

Unrelated note: the name ecashaddrjs is now pretty bad since it's been migrated to ts.

I will accept the changes after a rebase if:

  • Moving to a better solution requires a long delay, so we solve the immediate issue for users
  • This is only a temporary change.
This revision now requires changes to proceed.Sep 26 2024, 06:43

This needs a rebase after D16812.

Done

This diff solves a problem (chronik client install issues) and introduces a new one, as now the packages are no longer guaranteed in sync and ecashaddrjs can break all our apps silently.

I don't think we are introducing a new problem here. We want to make sure all of our apps are always using the latest version of every internal dependency. This is handled locally (and for packages that are not published, like Cashtab and ecash-herald) with relative installs. With the approach presented in this diff, we handle it on npm by replacing the relative install with the @latest install, i.e. the most-recently-published version on npm -- this would also be the most-recent version of this required dependency available to users of the module who install from npm.

It is possible that the most-recently-published version on npm is behind what we have locally. This would be an internal workflow problem; i.e. it means we made changes to a module that another module depends on but never published that module. In this case, the module could be broken. This is better than our current approach, where the module is guaranteed to be broken as the user will not have the same relative directory structure as the monorepo. It would be nice to have some kind of automatic solution for this, but at the moment I am not aware of one.

Example of the problem and how I would expect it to come up

Say we want to add new address features to chronik-client but they are not yet supported by ecashaddrjs. ecashaddrjs is where address lib features should live, so, in a diff, we update ecashaddrjs and update chronik-client to make use of these new features. Now if we land the diff...we publish both, but the "new" chronik-client might still end up consuming the old ecashaddrjs, and be broken. Solution: this kind of diff needs to be stacked and landed separately.

We need to move to workspaces which I expect to solve both problems instead.

I've played around with this and I don't think it offers a straightforward solution. With the current system -- relative installs -- we are guaranteed to always have the latest version locally. Workspaces or no workspaces, modules with relative installs will always be broken if they are published as such.

Workspaces offer some benefits like ensuring multiple repos are using the same dependencies and providing command-line shortcuts for managing multiple modules from the same top-level, e.g. we could npm test various repos from /home/bitcoin-abc instead of having to cd into, say, cashtab/; or we could npm i from the top level and this could ensure all modules have all of their dependencies installed (vs now where we must cd into each one and run npm i. So -- arguably we should add workspaces for these advantages. However this still wouldn't solve the npm publish problem.

Unrelated note: the name ecashaddrjs is now pretty bad since it's been migrated to ts.

True. I don't think the benefits of updating the name outweight the costs of deprecating the old name. There is no real functional difference between the two repos; npm tags modules with ts support as having it:

image.png (86×361 px, 7 KB)

Other larger and more-used repos have the same thing going on, so I think it's ok keeping the js name which implies (true) js support.

image.png (386×445 px, 38 KB)

I will accept the changes after a rebase if:

Moving to a better solution requires a long delay, so we solve the immediate issue for users
This is only a temporary change.

I would like to have a more robust way of handling this but at the moment I'm not aware of one. So, if we go the route suggested in this diff, it could be temporary for some time. We could, for example, use workspaces, and have no relative installs in the monorepo (instead everything comes from npm and is version pinned). Then, we could set up automation to publish all the modules every time a dependency changes -- for example, technically if we upgrade ecashaddrjs -- we should patch version bump and publish all our modules that depend on it, like chronik-client and ecash-lib. I think tho this is overkill. Existing users of chronik-client don't need the one with the latest version of ecashaddrjs unless chronik-client has been updated to make use of those features -- in which case it will have version bumped and published.

Advantages of relative installs

  • Everything in the monorepo is always on the same (most recent) version; all CI tests confirm this across multiple packages whenever anything changes
  • Repos that are not published but use published dependencies are protected from supply chain attacks on their monorepo deps (cashtab, ecash-herald, alias-server -- these are built from the monorepo and deployed without getting those packages from npm, imo a big win)

I think the relative installs are worth preserving until we have a clearly superior solution. workspaces offer some nice features but I do not see how they solve the problem (local package.jsons must have relative installs or version-specific installs of their own dependencies).

This revision is now accepted and ready to land.Oct 2 2024, 07:02