Replace Affjax with Fetch #657

CharlesTaylor7 · 2023-08-24T14:22:49Z

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 · 2023-08-24T22:19:02Z

app/spago.yaml

@@ -35,6 +34,7 @@ package:
    - integers
    - js-date
    - js-uri
+    - js-promise-aff


This is a transitive dependency of fetch, so it incurs no overhead.

CharlesTaylor7 · 2023-08-24T22:21:17Z

app/src/App/CLI/Git.purs

@@ -2,7 +2,6 @@ module Registry.App.CLI.Git where

 import Registry.App.Prelude

-import Affjax (URL)


URL is just a type alias for String, defined by Affjax.
Since the alias was used throughout the app, I've redefined the same alias in Registry.App.Prelude.

CharlesTaylor7 · 2023-08-24T22:24:46Z

app/src/App/Prelude.purs

-  -> Extra.Aff (RetryResult e b)
-withRetryRequest retry onAffjaxError onAffjaxResponse request =
-  withRetry retry (map (Either.either (Either.Left <<< onAffjaxError) onAffjaxResponse) (Affjax.Node.request request))
-


Lines 158-187, were lifted and rewritten for use in the module Fetch.Retry

app/src/Fetch/Retry.purs

CharlesTaylor7 · 2023-08-24T22:58:19Z

app/src/Fetch/Retry.purs

-      runRequest :: Aff CoreResponse.Response
-      runRequest = Promise.Aff.toAffE fetch
+  let runRequest :: Aff Response
+      runRequest = Fetch.fetch url r


The type signature of withRetryRequest was copied and adapted directly from the fetch source code. The only difference in these signatures is the return types.

The error message seems to be about ambiguous type variables, but I don't have a clue about to constrain the type any futher.

Error Message:

[1/1 NoInstanceFound] app/src/Fetch/Retry.purs:49:20 49 runRequest = Fetch.fetch url r ^^^^^^^^^^^^^^^ No type class instance was found for Prim.Row.Union input5 t1 ( body :: String , cache :: RequestCache , credentials :: RequestCredentials , headers :: Record t2 , integrity :: Integrity , method :: Method , mode :: RequestMode , referrer :: Referrer , referrerPolicy :: ReferrerPolicy ) The instance head contains unknown type variables. Consider adding a type annotation. while applying a function fetch of type Union @Type t0 t1 ( body :: String , cache :: RequestCache , credentials :: RequestCredentials , headers :: Record t2 , integrity :: Integrity , method :: Method , mode :: RequestMode , referrer :: Referrer , referrerPolicy :: ReferrerPolicy ) => Union @Type t3 t4 ( body :: RequestBody , cache :: String , credentials :: String , headers :: Headers , integrity :: String , method :: String , mode :: String , referrer :: String , referrerPolicy :: String ) => ToCoreRequestOptions t0 t3 => String -> Record t0 -> Aff { arrayBuffer :: ... , blob :: ... , body :: ... , headers :: Headers , json :: ... , ok :: Boolean , redirected :: Boolean , status :: Int , statusText :: String , text :: ... , url :: String } to argument url while inferring the type of fetch url in value declaration withRetryRequest where input5 is a rigid type variable bound at (line 0, column 0 - line 0, column 0) t0 is an unknown type t3 is an unknown type t1 is an unknown type t4 is an unknown type t2 is an unknown type

There's a relevant thread on the Discourse: https://discourse.purescript.org/t/specialize-function-via-annotation/3616. The fix there also applies here, this compiles

withRetryRequest :: forall input output thruIn thruOut headers . Union input thruIn (HighlevelRequestOptions headers String) => Union output thruOut CoreRequest.UnsafeRequestOptions => ToCoreRequestOptions input output => String -> { | input } -> Aff (RetryResult RetryRequestError Response) withRetryRequest url r = do let runRequest :: Aff Response - runRequest = fetch url r + runRequest = fetchWithProxy (Proxy :: Proxy thruIn) url r retryRequest runRequest + fetchWithProxy + :: forall input output thruIn thruOut headers + . Union input thruIn (HighlevelRequestOptions headers String) + => Union output thruOut CoreRequest.UnsafeRequestOptions + => ToCoreRequestOptions input output + => Proxy thruIn + -> String + -> { | input } + -> Aff Response + fetchWithProxy _ url r = do + request <- liftEffect $ new url $ Request.convert r + cResponse <- Promise.Aff.toAffE $ Core.fetch request + pure $ Response.convert cResponse

Visible type applications (https://github.com/purescript/purescript/releases/tag/v0.15.10) is a new feature in PureScript which lets us leave out the Proxy argument, so this also compiles

withRetryRequest :: forall input output thruIn thruOut headers . Union input thruIn (HighlevelRequestOptions headers String) => Union output thruOut CoreRequest.UnsafeRequestOptions => ToCoreRequestOptions input output => String -> { | input } -> Aff (RetryResult RetryRequestError Response) withRetryRequest url r = do let runRequest :: Aff Response - runRequest = fetchWithProxy (Proxy :: Proxy thruIn) url r + runRequest = fetch @thruIn url r retryRequest runRequest - fetchWithProxy - :: forall input output thruIn thruOut headers + fetch + :: forall input output @thruIn thruOut headers . Union input thruIn (HighlevelRequestOptions headers String) => Union output thruOut CoreRequest.UnsafeRequestOptions => ToCoreRequestOptions input output - => Proxy thruIn - -> String + => String -> { | input } -> Aff Response - fetchWithProxy _ url r = do + fetch url r = do request <- liftEffect $ new url $ Request.convert r cResponse <- Promise.Aff.toAffE $ Core.fetch request pure $ Response.convert cResponse

The only difference from the fetch source code is the @thruIn.

Sorry it doesn't help much with this PR, but it seems like it would be worthwhile to expose @thruIn in the fetch implementation upstream to use with VTAs (and maybe the other type parameters as well? not sure what the heuristic is).

We can add this to fetch so we can use vta here

We're using 0.15.10 in the registry so feel free to use VTAs if you'd like.

great. I have just tagged a new version (v3.0.0) of fetch with the vta change.

CharlesTaylor7 · 2023-08-24T23:04:44Z

@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 · 2023-08-25T13:55:26Z

@pete-murphy Thanks for the help!

@thomashoneyman
This is ready for review now.

foreign/package.json

thomashoneyman

This is a great start! Thanks for your work to puzzle out a good way to get fetch working with the retry function. I have a few comments, but overall this looks quite close.

app/src/App/Effect/Pursuit.purs

app/src/Fetch/Retry.purs

thomashoneyman · 2023-08-25T15:54:22Z

@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 [email protected]\n  <5.0.0 seen in [email protected]" ≠ "No versions found in the registry for does-not-exist in range\n  >=0.0.0 seen in [email protected] from declared dependencies transitive-broken\n  <5.0.0 seen in [email protected] 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 · 2023-08-25T18:24:55Z

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

CharlesTaylor7 · 2023-08-25T20:08:56Z

@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 · 2023-08-25T20:40:57Z

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 · 2023-08-26T13:37:52Z

app/src/App/Effect/Pursuit.purs

-      , timeout: Nothing
-      , url
+    result <- Run.liftAff $ Fetch.withRetryRequest url
+      { headers: { accept: "application/json" }


Should this be “Accept” the way it is above?

thomashoneyman · 2023-08-26T13:39:25Z

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

MonoidMusician · 2023-08-27T16:40:48Z

@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 · 2023-08-27T21:25:18Z

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.

CharlesTaylor7 changed the title ~~DRAFT - Fix~~ DRAFT - Replace Affjax with Fetch Aug 24, 2023

CharlesTaylor7 commented Aug 24, 2023

View reviewed changes

app/src/Fetch/Retry.purs Outdated Show resolved Hide resolved

CharlesTaylor7 commented Aug 24, 2023

View reviewed changes

CharlesTaylor7 marked this pull request as ready for review August 25, 2023 13:51

CharlesTaylor7 changed the title ~~DRAFT - Replace Affjax with Fetch~~ Replace Affjax with Fetch Aug 25, 2023

CharlesTaylor7 commented Aug 25, 2023

View reviewed changes

foreign/package.json Show resolved Hide resolved

pete-murphy mentioned this pull request Aug 25, 2023

Visible type application for thruIn? rowtype-yoga/purescript-fetch#9

Closed

thomashoneyman reviewed Aug 25, 2023

View reviewed changes

Swap out Affjax for Fetch

c8a2b4f

CharlesTaylor7 requested a review from thomashoneyman August 25, 2023 18:25

Set registry version to last passing version

aac2f69

thomashoneyman reviewed Aug 26, 2023

View reviewed changes

thomashoneyman approved these changes Aug 27, 2023

View reviewed changes

thomashoneyman merged commit 3509a47 into purescript:master Aug 27, 2023
15 checks passed

CharlesTaylor7 deleted the fetch-issue-643 branch August 28, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Affjax with Fetch #657

Replace Affjax with Fetch #657

CharlesTaylor7 commented Aug 24, 2023 •

edited

Loading

CharlesTaylor7 Aug 24, 2023 •

edited

Loading

CharlesTaylor7 Aug 24, 2023

CharlesTaylor7 Aug 24, 2023

CharlesTaylor7 Aug 24, 2023 •

edited

Loading

pete-murphy Aug 25, 2023 •

edited

Loading

sigma-andex Aug 25, 2023

thomashoneyman Aug 25, 2023

sigma-andex Aug 25, 2023

CharlesTaylor7 commented Aug 24, 2023 •

edited

Loading

CharlesTaylor7 commented Aug 25, 2023

thomashoneyman left a comment

thomashoneyman commented Aug 25, 2023

CharlesTaylor7 commented Aug 25, 2023

CharlesTaylor7 commented Aug 25, 2023

thomashoneyman commented Aug 25, 2023

thomashoneyman Aug 26, 2023

thomashoneyman commented Aug 26, 2023

MonoidMusician commented Aug 27, 2023

natefaubion commented Aug 27, 2023

		@@ -2,7 +2,6 @@ module Registry.App.CLI.Git where

		import Registry.App.Prelude

		import Affjax (URL)

Replace Affjax with Fetch #657

Replace Affjax with Fetch #657

Conversation

CharlesTaylor7 commented Aug 24, 2023 • edited Loading

CharlesTaylor7 Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

CharlesTaylor7 Aug 24, 2023

Choose a reason for hiding this comment

CharlesTaylor7 Aug 24, 2023

Choose a reason for hiding this comment

CharlesTaylor7 Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

pete-murphy Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

sigma-andex Aug 25, 2023

Choose a reason for hiding this comment

thomashoneyman Aug 25, 2023

Choose a reason for hiding this comment

sigma-andex Aug 25, 2023

Choose a reason for hiding this comment

CharlesTaylor7 commented Aug 24, 2023 • edited Loading

CharlesTaylor7 commented Aug 25, 2023

thomashoneyman left a comment

Choose a reason for hiding this comment

thomashoneyman commented Aug 25, 2023

CharlesTaylor7 commented Aug 25, 2023

CharlesTaylor7 commented Aug 25, 2023

thomashoneyman commented Aug 25, 2023

thomashoneyman Aug 26, 2023

Choose a reason for hiding this comment

thomashoneyman commented Aug 26, 2023

MonoidMusician commented Aug 27, 2023

natefaubion commented Aug 27, 2023

CharlesTaylor7 commented Aug 24, 2023 •

edited

Loading

CharlesTaylor7 Aug 24, 2023 •

edited

Loading

CharlesTaylor7 Aug 24, 2023 •

edited

Loading

pete-murphy Aug 25, 2023 •

edited

Loading

CharlesTaylor7 commented Aug 24, 2023 •

edited

Loading