purescript / registry-dev

Development work related to the PureScript Registry
https://github.com/purescript/registry
95 stars 80 forks source link

Replace Affjax with Fetch #657

Closed CharlesTaylor7 closed 1 year ago

CharlesTaylor7 commented 1 year ago

Resolves #643

Replaces Affjax with Fetch. I recommend reviewing Fetch.Retry first, the rest of changes are pretty mechanical. There's probably a case to be made for moving some or all of the Fetch.Retry module into the actual fetch package. Just let me know what you want to do.

CharlesTaylor7 commented 1 year ago

@thomashoneyman I've tried a few different approaches to reimplementing withRetryRequest and I could use some help. I'm running into very arcane type errors, and pretty much stuck at this point.

See these 2 commits for the two main approaches I tried:

On the bright side, I was able to port the rest of the application to use the new fetch based api, just haven't figured out how to implement the retry helper in a way to satisfy the type checker.

CharlesTaylor7 commented 1 year ago

@pete-murphy Thanks for the help!

@thomashoneyman This is ready for review now.

thomashoneyman commented 1 year ago

@MonoidMusician A bit of an odd error in the tests, coming from the solver:

   ✗ Nested dependency of target package has no versions in range.:

  "No versions found in the registry for does-not-exist in range\n  >=0.0.0 seen in fixed-broken@2.0.0\n  <5.0.0 seen in fixed-broken@2.0.0" ≠ "No versions found in the registry for does-not-exist in range\n  >=0.0.0 seen in fixed-broken@2.0.0 from declared dependencies transitive-broken\n  <5.0.0 seen in fixed-broken@2.0.0 from declared dependencies transitive-broken"

The only thing that would affect the solver here are dependency updates; I know that there were recently changes to ordered-collections — do you see anything that would be causing this change to the error message?

CharlesTaylor7 commented 1 year ago

@thomashoneyman Alright, I resolved the comments you left & squashed down to 1 commit.

CharlesTaylor7 commented 1 year ago

@thomashoneyman I manually tested the one failing test against different registry versions, and found the last version that works is 35.1.0, and the first failing version is 35.2.0.

thomashoneyman commented 1 year ago

Looks like that corresponds with ordered-collections being updated to 3.1.0: https://github.com/purescript/registry/blob/main/package-sets/35.2.0.json

Release is here, looks like a pretty major rewrite of the internals: https://github.com/purescript/purescript-ordered-collections/releases/tag/v3.1.0

I'll have to review separately and see if the change to behavior is benign or not.

thomashoneyman commented 1 year ago

The code looks good — I’ll give this a try later today and hopefully we can get it merged!

MonoidMusician commented 1 year ago

@thomashoneyman I don't see an obvious reason why the error message would have changed, assuming both the old Data.Map and new one are both correct. The API surface I used is pretty small. Maybe traversal order changed somewhere? But it's a pretty benign change to that error message so I don't think it's a big deal. The algorithm should still be correct.

natefaubion commented 1 year ago

Maybe traversal order changed somewhere?

This is certainly possible. Both old and new traverse order effects structurally (ie, equivalent to toUnfoldableUnordered), and there's no reason to assume that the trees internally would produce the same effect order.