search

clj-commons / manifold

A compatibility layer for event-driven abstractions
1.01k stars 106 forks source link

Use `bound-fn` in `let-flow` #202

Closed tanzoniteblack closed 2 years ago

tanzoniteblack commented 3 years ago

bound-fn preserves bindings of dynamic variables, even if the function ends up being executed on a different thread.

I've made sure to test that without this change, the test I've added to detect dynamic variables across executor threads actually does still fail.

tanzoniteblack commented 3 years ago

Backport of https://github.com/yummly/manifold/pull/2 , which I briefly mentioned in https://github.com/clj-commons/manifold/issues/183

KingMob commented 2 years ago

@tanzoniteblack I recently updated Manifold's indentation, partly to fix some inconsistencies, partly to modernize it with current styles. This caused some conflicts, which I fixed.

As part of that, I also took a closer look at the new test for bound-fn in let-flow. If I'm not mistaken, the code to create the executor was taken from how manifold.executor sets up its default pools. manifold.executor uses a complicated delay/promise setup to avoid creating the executors until/unless they're actually used, but since we know we need an executor for the test, we can create it directly.

I went ahead and simplified the test code, as well as verifying the test results were the same, both before and after the bound-fn change, but let me know if you think I missed something.

tanzoniteblack commented 2 years ago

Looks good, @KingMob