-
Notifications
You must be signed in to change notification settings - Fork 19
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
Internal -> Buffer; invert MonadBuffer module dependency #51
Conversation
- All `Internal` FFI has been moved into `Node.Buffer` and rewritten to use `FnX`/`EffectFnX` code - ST.purs was rewritten to reuse the `Node.Buffer` code, coercing the Effect to ST since they are the same - the `MonadBuffer` class was originally re-exported by ST/Buffer alike. This module dependency has been inverted. The main purpose behind this was to improve type inference for things like `Buffer.toString buf UTF8`
@thomashoneyman This is probably good for an initial review. Things to consider:
|
There's another issue that came to mind recently. The identifiers we use in our bindings don't always make it clear which Node API is being used in the FFI. In other words, if someone wanted to look at the docs for a given PS identifier, they would need to look at the source code, look at the FFI source code, and then look at the corresponding Node docs for that identifier. |
Would you mind describing how these breaking changes would affect a typical user of this library? Unless I’m explicitly using the MonadBuffer class, is there anything I’d need to do? |
If one is using one of the data constructors from |
Anupam looked at this code and suggested not inverting the module dependency for the -create :: forall m b. MonadBuffer b m => m b
+create :: Effect Buffer In other words, Also, I think for ease of review, this PR should be broken up into smaller ones (you know, actually "do the right thing" when it comes to contributing to open source). |
@thomashoneyman See #52 |
This PR contains three changes:
First. it fixes #34 by updating all FFI to use uncurried functions (e.g.
FnX
/EffectFnX
) ().Second, it improves type inference by exposing the API otherwise exposed via the
MonadBuffer
type class in favor of hardcoding the corresponding values forBuffer
andST
in their respective modules (while still implementing everything only once via theBuffer.purs
file). To reduce breakage, theirMonadBuffer
instances were moved into theNode/Buffer/Class.purs
file.One implication of the second change is that the module dependency has been inverted. Whereas before
Node/Buffer.purs
depended onNode/Buffer/Class.purs
(i.e. it exposed the Buffer API by re-exported the class and its members), nowNode/Buffer/Class.purs
depends onNode/Buffer.purs
to import theBuffer
type used in its API.Third, the
nullable
dependency was added since usingtoMaybe
is simpler than passing in theNothing
andJust
constructors into an FFI and handling the null/undefined possibility in FFI.Fourth, adds APIs that were missing when I checked with the Node 18 docs. There's likely a few more here and there (e.g. variants of functions if one passes in args of a different type), but I didn't bother to add those.
Checklist: