Migrate from ContT to Aff #208
Conversation
| runEffectFn1 setAnnotations (mapMaybe toAnnotation warnings_) | ||
| for_ sources (execute (JS js)) | ||
| Right (CompileFailed (FailedResult { error })) -> do | ||
| showJs <- liftEffect isShowJsChecked |
There was a problem hiding this comment.
These changes are largely indentation related; the only real difference is the introduction of liftEffect and switching runContT for binds in Aff.
| Right { status } | status >= StatusCode 400 -> | ||
| pure $ Left $ "Received error status code: " <> show status | ||
| Right { body } -> | ||
| pure $ Right body |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The code seems fine to me as-is if the check is necessary. If it's not necessary, I think we should remove it.
There was a problem hiding this comment.
@hdgarrood Yep, it turns out this check is necessary.
If the request is made with a malformed URL:
If the request returns a 404:
| Right { status } | status >= StatusCode 400 -> | ||
| pure $ Left $ "Received error status code: " <> show status | ||
| Right { body } -> | ||
| pure $ Right $ runExcept (decode (unsafeToForeign body)) |
There was a problem hiding this comment.
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).
| 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." |
There was a problem hiding this comment.
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.
|
One last thing: I needed to symlink the |
| } | ||
|
|
||
| exports.tryLoadFileFromGist_ = function(gistInfo, filename, done, fail) { | ||
| exports.rawUrl_ = function (gistInfo, filename) { |
There was a problem hiding this comment.
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.
| Right { status } | status >= StatusCode 400 -> | ||
| pure $ Left $ "Received error status code: " <> show status | ||
| Right { body } -> | ||
| pure $ Right body |
There was a problem hiding this comment.
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.
|
@hdgarrood I've addressed your feedback |
This PR addresses the first part of #199 by switching from
ContTand the FFI for requests toAffand Affjax for requests throughout the application. I tried to make the minimum changes required to switch.The
Main.pursandLoader.pursfiles have largely mechanical changes, and you can verify at a glance that the changes come down to a fewliftEffects and switchingrunContTfor Aff binds and the occasionallaunchAff_.However, the
API.pursandGist.purshave 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.