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 with multiple let bindings #30

Closed
jo-sm opened this issue May 24, 2023 · 0 comments
Closed

Fix async with multiple let bindings #30

jo-sm opened this issue May 24, 2023 · 0 comments
Labels
bug Something isn't working

Comments

@jo-sm
Copy link
Contributor

jo-sm commented May 24, 2023

async isn't working with multiple let bindings if any later binding uses an earlier one:

(async
  (let [something "yes"
        result (await (js/Promise.resolve something))]
    ...)

This will fail because we internally turn that into Promise.all, so we need to be smarter and handle each binding based on if they are await-ed or not.

@jo-sm jo-sm added the bug Something isn't working label May 24, 2023
jo-sm added a commit that referenced this issue Jun 1, 2023
Previously, the `async` macro would attempt to treat a non-symbol form as a function call
and so the following would result:

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

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

This commit fixes this so that let bindings are properly created.

- 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 #30, #31.
jo-sm added a commit that referenced this issue Jun 1, 2023
Previously, the `async` macro would attempt to treat a non-symbol form as a function call
and so the following would result:

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

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

This commit fixes this so that let bindings are properly created.

- 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 #30, fixes #31.
jo-sm added a commit that referenced this issue Jun 1, 2023
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 #30, fixes #31.
jo-sm added a commit that referenced this issue Jun 1, 2023
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 #30, fixes #31.
@jo-sm jo-sm closed this as completed in b457de2 Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

1 participant