Use EffectFnX

[colinwahl]

Description of the change

Removes the unsafe mkEffect utility in favor of using EffectFn throughout.

Intends to close #28

---

Checklist:

• Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
• Linked any existing issues or proposals that this pull request should close
  - [ ] Updated or added relevant documentation
  - [ ] Added a test for the contribution (if applicable)

[JordanMartinez]

Should https://github.com/purescript-node/purescript-node-fs/pull/70/files#diff-3bac72693cd408f833d773e883ef64a1271ff4d71f7345744cd76fa64e458af0R59-R63 also be updated to use EffectFnX?

[colinwahl]

That link isn't expanding for me - are you talking about handleCallbackImpl and related types/utils?

I didn't in this pass because they were never used with mkEffect and I wanted to minimize the changes to just removing that unsafe util.

[JordanMartinez]

That link isn't expanding for me - are you talking about handleCallbackImpl and related types/utils?

I didn't in this pass because they were never used with mkEffect and I wanted to minimize the changes to just removing that unsafe util.

Yes. And even if it's not a problem now, I think that will be the next issue that arises if using this with purs-backend-es. So, would that be fixed in a later PR?

[JordanMartinez]

Besides the handle callback question, this LGTM.

[colinwahl]

After a bit more thought, I've realized the callbacks shouldn't cause any issues with purs-backend-es.

The other effectful functions were a problem because we were using unsafeCoerce and assuming that the runtime representation of effects were Unit -> a - but callbacks here are represented as Fn3 and use runFn3 to construct a curried representation which is handled fine by the optimizer.

Can you think of something specific that you think is wrong there that I'm missing?

[JordanMartinez]

When I look at the FFI for it (https://github.com/purescript-node/purescript-node-fs/blob/master/src/Node/FS/Async.js#L27-L35), it looks like an EffectFnX, not an FnX. The real issue may be JSCallback.

Idk. I think this could be an issue, but if you can test it and show it's not, then I'm just wrong 😄

[colinwahl]

Hmm, yeah, I could see the argument that JSCallback should be an EffectFn2, since it does execute an effect (the Callback) after all.

I can change it over if you'd like! Luckily Fn2 and EffectFn2 generate the same code so I think it's a safe change to make.

[garyb]

I'm not sure what's more right here either... it does seem like it should be an EffectFnX, but I'm not quite sure how we're getting away with the implementation as it is right now without there already being an Effect somewhere 😄. I'm probably just not seeing the full picture correctly from what I looked at.

[colinwahl]

I think that JSCallback should have been an EffectFn2, and after doing that change I realized that I could write all of handleCallback in PureScript, instead of having the FFI definition handleCallbackImpl.

I think that's quite the improvement - I just pushed up a commit with those changes.

[garyb]

Nice!

[colinwahl]

Thanks for the reviews! I don't know the procedure for merging is within purescript-node - let me know if there's anything more I need to do.