-
Notifications
You must be signed in to change notification settings - Fork 131
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
Update Node dependencies to latest breaking ones #1122
Comments
We can get this done by having two PRs open at the same time:
|
Honestly, I think Spago should rely on registry-lib but the registry application shouldn't rely on Spago and should just have its own implementation of the yaml-to-manifest conversion. It's way simpler than this tie-the-knot scenario we're in, and it allows the registry to be able to support multiple variants on the spago.yaml file in the case that it changes — it's in alpha, after all. The registry has to be able to support any changes because now packages have been published with spago.yaml files in their repositories. If we keep them coupled then it becomes impossible to update any shared dependency without this coordinated dance between the repositories. |
It's not clear to me why things are even structured this way. And once explained, I'd ask if there was some other way to decouple these things. So, why is the code structured this way, and what's the tradeoff we're making here by doing it that way? |
The registry is importing a piece of Spago so that it can decode manifests without reimplementing that code, and Spago is importing the Registry codebase to avoid reimplementing a big big chunk of code. If we don't want to have this structure we could split the codebases, e.g. by having the registry reimplement what it needs, or we could join them together so they move at the same time. |
I think it's a definite advantage that package managers like Spago are able to import types like On the face of it it's also an advantage that the registry app can import Spago's manifest type and parsing. The disadvantage is that it's not possible to update any shared dependency without managing releases as you described above, which involves multiple people coordinating pull requests and branches, with CI broken in the intermediate steps. It isn't impossible, but it's an irritating process to go through. Since both use package sets we are basically frozen to a particular package set unless we do this dance. If spago imports registry-lib only then the code bases can be updated at their own pace. The registry can update and spago can catch up at its own pace, and the registry doesn't need to worry about spago updates unless they touch the manifest format. This may feel like a disadvantage — don't we want Spago and the registry to be perfectly in sync? — but in truth we can't guarantee they are in sync anyway. If Spago changes the manifest format, for example, and the registry updates to it without including compatibility code for working with older manifests, then the registry essentially drops support for the older manifest format. Presumably we can trust that Spago will include the compatibility code itself but it would be nicer for the registry to be explicit. This all hinges on whether or not it's easy to reimplement the Spago manifest code and whether or not it will change frequently. It seems to me that it's not bad: The registry only cares about a few fields of the Lines 72 to 75 in 6d5929a
Lines 95 to 110 in 6d5929a
The registry also needs to convert YAML to JSON, which Spago does via the With all this in mind I suggest that we:
With this done, doing something like switching from |
Sure, this sounds good! |
What does this look like? |
Unless this is an issue from Spago's perspective already I say we defer that one for the future. |
Done in #1129 |
This issue is blocking #1048 and #1000.
To update Spago here, I need to update
registry-dev
libraries. However,registry-dev
libs depend onspago/core
, which depends onnode-fs-aff
. Even if I workaround this in theregistry-dev
libs by overridingspago/core
'sdependencies
with my own, the fact remains that it's using old Node libraries, so compilingcore
on the Registry side fails with this error. See purescript/registry-dev#671 for more context:because
Process.exit
was changed in the Node libs:The text was updated successfully, but these errors were encountered: