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.

Sign up for GitHub

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

Jump to bottom

followup #16871 asyncjs.then: allow pipelining procs returning futures #17189

Merged
merged 15 commits into from
Mar 4, 2021
Merged

followup #16871 asyncjs.then: allow pipelining procs returning futures #17189

merged 15 commits into from
Mar 4, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Feb 25, 2021 •
edited
Loading

followup #16871

  • then now allows pipelining futures (instead of just callbacks), and we avoid returning Future[Future[T]] which would be wrong (would break the monadic property that allows pipelining
  • increase test coverage, eg then with onReject callback is now tested
  • factored the 3 then overloads into a single proc, which is cleaner for docs etc (in fact it would've been 6 overloads with the added support for futures, until i figured out how to use a single non-overloaded proc)
  • introduces typeOrVoid (which I'll expose in typetraits in future work)

future work

  • expose typeOrVoid or make typeof(someProcReturningVoid()) is void work
  • remove nimExperimentalAsyncjsThen flag once stabilized

@timotheecour timotheecour mentioned this pull request Feb 26, 2021
Merged
@timotheecour timotheecour marked this pull request as ready for review February 26, 2021 04:40
ringabout
ringabout reviewed Feb 26, 2021
View reviewed changes
lib/js/asyncjs.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_16871_followup_asyncjs_then branch from 12ac0fe to 006b55b Compare February 26, 2021 07:54
@timotheecour timotheecour mentioned this pull request Feb 26, 2021
Merged
@timotheecour timotheecour requested a review from Araq February 27, 2021 20:02
@timotheecour
Copy link
Member Author

timotheecour commented Feb 27, 2021 •
edited
Loading

ping @Araq after this PR, fetch (from #12531) works [1]

# funs.nim
from std/jsffi import JsObject
import std/[jsfetch,asyncjs,sugar,jsconsole]
proc main {.async.} =
  await fetch("https://api.github.com/users/manishmshiva".cstring)
    .then((response: Response) => response.json())
    .then((json: JsObject) => console.log(json))
    .catch((err: Error) => console.log("Request Failed", err))

  await fetch("https://D20210227T115923_bad_url".cstring)
    .then((response: Response) => response.json())
    .then((json: JsObject) => console.log(json))
    .catch((err: Error) => console.log("Request Failed from nim", err))
discard main()

which allows easy port from js code like this https://www.freecodecamp.org/news/javascript-fetch-api-tutorial-with-js-fetch-post-and-header-examples/

// GET Request.
fetch('https://api.github.com/users/manishmshiva')
    // Handle success
    .then(response => response.json())  // convert to json
    .then(json => console.log(json))    //print data to console
    .catch(err => console.log('Request Failed', err)); // Catch errors

in a broswer, the following html:

<!DOCTYPE html>
<html>
<script src="funs.js"></script>
<body>
</body>
</html>

shows:
image

[1] I had to modify #12531 as follows /cc @juancarlospaco

diff --git a/lib/std/jsfetch.nim b/lib/std/jsfetch.nim
index 93ae44261..cf310174b 100644
--- a/lib/std/jsfetch.nim
+++ b/lib/std/jsfetch.nim
@@ -72,7 +72,7 @@ func clone*(self: Response or Request): Response {.importjs: "#.$1()".}
 proc text*(self: Body or Request): Future[cstring] {.importjs: "#.$1()".}
   ## https://developer.mozilla.org/en-US/docs/Web/API/Body/text

-proc json*(self: Body): Future[JsObject] {.importjs: "#.$1()".}
+proc json*(self: Response): Future[JsObject] {.importjs: "#.$1()".}
   ## https://developer.mozilla.org/en-US/docs/Web/API/Body/json

 proc formData*(self: Body): Future[FormData] {.importjs: "#.$1()".}

@juancarlospaco
Copy link
Collaborator

It is a relevant change, therefore needs another line on the changelog. IMHO.

@timotheecour
Copy link
Member Author

timotheecour commented Feb 27, 2021 •
edited
Loading

I think it's already covered by the existing one:

- Added `then`, `catch` to `asyncjs`, for now hidden behind `-d:nimExperimentalAsyncjsThen`.

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Feb 27, 2021
@timotheecour timotheecour mentioned this pull request Mar 3, 2021
Merged
@timotheecour timotheecour force-pushed the pr_16871_followup_asyncjs_then branch from 6eb67e1 to bb26bd1 Compare March 4, 2021 09:16
@timotheecour timotheecour mentioned this pull request Mar 4, 2021
Open
@timotheecour timotheecour requested a review from dom96 March 4, 2021 10:15
@Araq Araq merged commit a66637b into nim-lang:devel Mar 4, 2021
@timotheecour timotheecour deleted the pr_16871_followup_asyncjs_then branch March 4, 2021 15:28
ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
@timotheecour @ringabout
followup nim-lang#16871 asyncjs.then: allow pipelining procs returnin…
d1683d5 
…g futures (nim-lang#17189)

* followup nim-lang#16871 asyncjs.then: allow pipelining procs returning futures
* rename test files where they belong
* fix tests
* tests for then with `onReject` callback
* rename test file containing fail to avoid messing with grep
* address comments
* cleanup
* un-disable 1 test
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@timotheecour @ardek66
followup nim-lang#16871 asyncjs.then: allow pipelining procs returnin…
3444a3e 
…g futures (nim-lang#17189)

* followup nim-lang#16871 asyncjs.then: allow pipelining procs returning futures
* rename test files where they belong
* fix tests
* tests for then with `onReject` callback
* rename test file containing fail to avoid messing with grep
* address comments
* cleanup
* un-disable 1 test
@metagn metagn removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ringabout ringabout ringabout approved these changes

@Araq Araq Awaiting requested review from Araq

@dom96 dom96 Awaiting requested review from dom96

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timotheecour @juancarlospaco @ringabout @Araq @metagn