Migrate from ContT to Aff by thomashoneyman · Pull Request #208 · purescript/trypurescript

thomashoneyman · 2020-12-15T07:22:20Z

This PR addresses the first part of #199 by switching from ContT and the FFI for requests to Aff and Affjax for requests throughout the application. I tried to make the minimum changes required to switch.

The Main.purs and Loader.purs files have largely mechanical changes, and you can verify at a glance that the changes come down to a few liftEffects and switching runContT for Aff binds and the occasional launchAff_.

However, the API.purs and Gist.purs have larger changes because I've switched to use Affjax instead of relying on the FFI for these requests. In those files, you can see that the parameters used for the GET or POST requests have been copied almost exactly into the equivalent Affjax code. I'm leaving a few review comments below to clear up some points that may be confusing at a glance.

I've tested Try PureScript locally using these changes and it continues to work as before. However, I haven't tested saving a gist -- that already doesn't work on Try PureScript today, and I don't think my changes make it any more broken than it was before. A subsequent PR as part of the changes in #199 ought to fix the gist saving issue, as @milesfrain has already demonstrated on try.ps.ai.

thomashoneyman

I've left some additional clarifying comments.

thomashoneyman · 2020-12-15T07:23:26Z

client/src/Main.purs

-                       runEffectFn1 setAnnotations (mapMaybe toAnnotation warnings_)
-                     for_ sources (execute (JS js))
-          Right (CompileFailed (FailedResult { error })) -> do
+            showJs <- liftEffect isShowJsChecked


These changes are largely indentation related; the only real difference is the introduction of liftEffect and switching runContT for binds in Aff.

thomashoneyman · 2020-12-15T07:25:51Z

client/src/Try/API.purs

+  Right { status } | status >= StatusCode 400 ->
+    pure $ Left $ "Received error status code: " <> show status
+  Right { body } ->
+    pure $ Right body


I believe the second check (which guards against status codes over 400) is technically unnecessary, but I couldn't verify that at a quick breeze through the Affjax documentation so I've included it here. I can remove these checks if they're redundant, though.

This doesn't really need to be in ExceptT, but doing so allows me to minimize the diff everywhere else.

Can you check and see what happens if you get a 404 status code? I haven't been able to verify from a quick glance through the documentation either but I actually suspect it's not redundant. Either way, I think we should know the answer before merging.

I’ll verify this check is necessary. Do you have any opinion on the code as-is if the check does turn out to be necessary?

The code seems fine to me as-is if the check is necessary. If it's not necessary, I think we should remove it.

@hdgarrood Yep, it turns out this check is necessary.

If the request is made with a malformed URL:

If the request returns a 404:

thomashoneyman · 2020-12-15T07:26:51Z

client/src/Try/API.purs

+  Right { status } | status >= StatusCode 400 ->
+    pure $ Left $ "Received error status code: " <> show status
+  Right { body } ->
+    pure $ Right $ runExcept (decode (unsafeToForeign body))


This unsafeToForeign is for parity with the previous FFI code, which specified a JSON return type (in jQuery) and then decodes it as Foreign (from PureScript).

thomashoneyman · 2020-12-15T07:28:58Z

client/src/Try/Gist.purs

+      case Object.lookup "id" obj of
+        Just v | Just v' <- toString v -> Right v'
+        Nothing -> Left "No id key found."
+        _ -> Left "Key id was not a string."


This seemed like the simplest way to pull out the id key from an object in the response without resorting to the FFI and without writing a new decoder. Like the rawUrl_ FFI code I think ideally we'd move to something like codec-argonaut in the future and replace this with a proper representation of the response from the API.

thomashoneyman · 2020-12-15T07:30:43Z

One last thing: I needed to symlink the staging/output directory into public/js/output in order for Try PureScript to pick up dependencies. Simply running spago install and then stack exec trypurescript 8081 $(spago sources) wasn't enough (as per the dev instructions in the README). Just a FYI for anyone testing this out locally.

thomashoneyman · 2020-12-15T07:39:28Z

client/src/Try/Gist.js

-}
-
-exports.tryLoadFileFromGist_ = function(gistInfo, filename, done, fail) {
+exports.rawUrl_ = function (gistInfo, filename) {


This small function, rawUrl_, is copied from the beginning of tryLoadFileFromGist_:

exports.tryLoadFileFromGist_ = function(gistInfo, filename, done, fail) { if (gistInfo.files && gistInfo.files.hasOwnProperty(filename)) { var url = gistInfo.files[filename].raw_url;

I opted to keep this code in the FFI rather than implement new data types and decoding in PureScript. That's once again to keep the diff minimal. In the future, though, it would be nice to switch to something like codec-argonaut instead. You can see this function used in tryLoadFileFromGist in the PureScript code.

client/src/Main.purs

client/src/Try/Gist.purs

hdgarrood · 2020-12-16T01:53:57Z

client/src/Try/API.purs

+  Right { status } | status >= StatusCode 400 ->
+    pure $ Left $ "Received error status code: " <> show status
+  Right { body } ->
+    pure $ Right body


Can you check and see what happens if you get a 404 status code? I haven't been able to verify from a quick glance through the documentation either but I actually suspect it's not redundant. Either way, I think we should know the answer before merging.

thomashoneyman · 2020-12-16T02:25:26Z

@hdgarrood I've addressed your feedback

hdgarrood

Nice 🎉

thomashoneyman added 5 commits December 12, 2020 15:08

Add Aff to project dependencies

d918289

Switch API calls from ContT to Aff

b6f9d51

Switch Gist code from ContT to Aff

552acb7

Remove accidentally-committed Try PureScript file

3d26261

Tweaks

e6efb9a

thomashoneyman commented Dec 15, 2020

View reviewed changes

Move rawUrl_ closer to prior implementation

f0f87b9

thomashoneyman commented Dec 15, 2020

View reviewed changes

milesfrain approved these changes Dec 15, 2020

View reviewed changes

JordanMartinez approved these changes Dec 15, 2020

View reviewed changes

hdgarrood reviewed Dec 16, 2020

View reviewed changes

thomashoneyman added 2 commits December 15, 2020 18:07

Remove type synonyms

7c260e6

Remove unnecessary callbacks in Main

2d4d25a

hdgarrood approved these changes Dec 16, 2020

View reviewed changes

thomashoneyman merged commit a837f01 into master Dec 16, 2020

thomashoneyman deleted the contt-to-aff branch January 25, 2021 21:55

thomashoneyman mentioned this pull request Mar 27, 2021

Convert from ContT and Ajax FFI to Aff and Affjax #179

Closed

Conversation

thomashoneyman commented Dec 15, 2020

Uh oh!

thomashoneyman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomashoneyman commented Dec 15, 2020

Uh oh!

thomashoneyman Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomashoneyman commented Dec 16, 2020

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomashoneyman Dec 15, 2020 •

edited

Loading