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

Update Node dependencies to latest breaking ones #1122

Closed
JordanMartinez opened this issue Nov 17, 2023 · 9 comments
Closed

Update Node dependencies to latest breaking ones #1122

JordanMartinez opened this issue Nov 17, 2023 · 9 comments
Assignees

Comments

@JordanMartinez
Copy link
Contributor

This issue is blocking #1048 and #1000.

To update Spago here, I need to update registry-dev libraries. However, registry-dev libs depend on spago/core, which depends on node-fs-aff. Even if I workaround this in the registry-dev libs by overriding spago/core's dependencies with my own, the fact remains that it's using old Node libraries, so compiling core on the Registry side fails with this error. See purescript/registry-dev#671 for more context:

[1/3 TypesDoNotUnify] .spago/packages/spago-core/47b735c5845a084f40826ae1989c3dc6c9171f21/core/src/Log.purs:161:23

  161    Effect.liftEffect $ Process.exit 1
                             ^^^^^^^^^^^^^^

  Could not match type
    Effect
  with type
    Function Int
  while trying to match type Effect t1
    with type Int -> t2
  while checking that expression exit 1
    has type Effect t0
  in value declaration die'
  where t0 is an unknown type
        t1 is an unknown type
        t2 is an unknown type

because Process.exit was changed in the Node libs:

-Process.exit :: Effect Unit
+Process.exit' :: Int -> Effect Unit
@f-f
Copy link
Member

f-f commented Nov 18, 2023

We can get this done by having two PRs open at the same time:

  • fix spago-core in spago/branch-1 - CI will be broken, but we only care about having spago-core compile fine
  • then point at that commit from spago/branch-1 in registry-dev/update-node-deps, so we get past the issue
  • once the registry-lib compiles happily, we can point this spago/branch-1 to refer to the registry-dev/update-node-deps branch for the registry source, get everything to compile, merge it
  • ..at which point the registry-dev/update-node-deps can point to the tip of spago's trunk again, and we can merge that in too

@thomashoneyman
Copy link
Member

thomashoneyman commented Nov 19, 2023

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.

@JordanMartinez
Copy link
Contributor Author

We can get this done by having two PRs open at the same time:

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?

@f-f
Copy link
Member

f-f commented Nov 21, 2023

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 don't mind either but I also don't mind having things set up this way - why is it a big deal?

@thomashoneyman
Copy link
Member

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 don't mind either but I also don't mind having things set up this way - why is it a big deal?

I think it's a definite advantage that package managers like Spago are able to import types like ManifestIndex and functions like solve from the registry, and I definitely think the code bases should be coupled in that respect. Ideally we would limit how many dependencies Spago incurs from this lib; we've probably been too liberal about using node-* libraries in the library.

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 package section and publish sub-section of the config, which are each already using registry types:

type PackageConfig =
{ name :: PackageName
, description :: Maybe String
, dependencies :: Dependencies

spago/core/src/Config.purs

Lines 95 to 110 in 6d5929a

type PublishConfig =
{ version :: Version
, license :: License
, location :: Maybe Location
, include :: Maybe (Array FilePath)
, exclude :: Maybe (Array FilePath)
}
publishConfigCodec :: JsonCodec PublishConfig
publishConfigCodec = CA.object "PublishConfig"
$ CA.recordProp (Proxy :: _ "version") Version.codec
$ CA.recordProp (Proxy :: _ "license") License.codec
$ CA.recordPropOptional (Proxy :: _ "location") Location.codec
$ CA.recordPropOptional (Proxy :: _ "include") (CA.array CA.string)
$ CA.recordPropOptional (Proxy :: _ "exclude") (CA.array CA.string)
$ CA.record

The registry also needs to convert YAML to JSON, which Spago does via the yaml library (and which the registry could do as well with a similarly-thin binding).

With all this in mind I suggest that we:

  1. Reimplement spago.yaml parsing in the registry (just the sections we care about)
  2. Remove the dependency on spago-core in the registry
  3. Consider trimming dependencies in registry-lib to minimize how many packages spago brings in via the library.

With this done, doing something like switching from codec-argonaut to codec-json can be done by updating the registry and then spago. The alternative is for any update like this to update registry-lib (leaving the app broken), then spago, then the registry-app, managing package set overrides along the way.

@f-f
Copy link
Member

f-f commented Nov 22, 2023

Sure, this sounds good!

@JordanMartinez
Copy link
Contributor Author

Consider trimming dependencies in registry-lib to minimize how many packages spago brings in via the library.

What does this look like?

@thomashoneyman
Copy link
Member

Unless this is an issue from Spago's perspective already I say we defer that one for the future.

@f-f
Copy link
Member

f-f commented Jan 14, 2024

Done in #1129

@f-f f-f closed this as completed Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants