search

purescript-node / purescript-node-fs

Node.js file I/O for purescript
MIT License
33 stars 34 forks source link

Use EffectFnX #70

Closed colinwahl closed 2 years ago

colinwahl commented 2 years ago

Description of the change

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

Intends to close #28

Checklist:

JordanMartinez commented 2 years ago

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

colinwahl commented 2 years ago

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 commented 2 years ago

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?

colinwahl commented 2 years ago

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 commented 2 years ago

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 :smile:

colinwahl commented 2 years ago

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 commented 2 years ago

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 :smile:. I'm probably just not seeing the full picture correctly from what I looked at.

colinwahl commented 2 years ago

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.

colinwahl commented 2 years ago

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.