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.

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

Fix async let binding expansion #35

Merged
merged 4 commits into from
Jun 9, 2023
Merged

Fix async let binding expansion #35

merged 4 commits into from
Jun 9, 2023

Conversation

jo-sm
Copy link
Contributor

@jo-sm jo-sm commented Jun 1, 2023

This PR fixes let binding usage inside of async, so that you can properly use await for let bindings.

  • Handle complex let bindings, like using an earlier binding in a later one.

@jo-sm jo-sm requested a review from ottonascarella June 1, 2023 16:55
@jo-sm jo-sm force-pushed the async-issues branch 2 times, most recently from 6321f5b to e693308 Compare June 1, 2023 16:59
@jo-sm jo-sm marked this pull request as draft June 1, 2023 17:11
jo-sm added 3 commits June 5, 2023 13:51
Previously, the `async` macro would attempt to treat a non-symbol value for a binding
in a `let` as a function call and so the following would happen:

```
(h/async
 (let [result (await (js/Promise.resolve "value"))]
  ...))

;; Compiles into...
var xyz = Promise.resolve("value").call()
```

This commit fixes this so that `await`-ed `let` bindings are properly handled.

- Remove macroexpansion from tests

With the correct let binding handling, macroexpansion doesn't work because the resulting
binding names are random (something like v_51003, p_40892), which can't really be equated.
Since we can test the `async` macro directly we can just remove the macroexpansion tests
completely.

Fixes #31.
Previously, if you had a let binding that depended on a previous binding,
it wouldn't work because the `async` macro would expand into a `js/Promise.all`
call with the values, and the symbols as the bindings for the next `.then` call.
This meant that if one of the values in `Promise.all` depended on a previous
binding symbol, it wouldn't be able to resolve it. This commit fixes this by
generating a new `let` call for each pair if any of the binding pairs has
`await`.

Fixes #30.
@jo-sm jo-sm marked this pull request as ready for review June 5, 2023 16:09
In the future, it might be nice to bring this back, but I would like to
find a better way to handle it.
@jo-sm jo-sm merged commit ef9ca2a into master Jun 9, 2023
@jo-sm jo-sm deleted the async-issues branch June 9, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant