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

Internal -> Buffer; invert MonadBuffer module dependency #51

Closed
wants to merge 11 commits into from

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented Jun 23, 2023

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 for Buffer and ST in their respective modules (while still implementing everything only once via the Buffer.purs file). To reduce breakage, their MonadBuffer instances were moved into the Node/Buffer/Class.purs file.

One implication of the second change is that the module dependency has been inverted. Whereas before Node/Buffer.purs depended on Node/Buffer/Class.purs (i.e. it exposed the Buffer API by re-exported the class and its members), now Node/Buffer/Class.purs depends on Node/Buffer.purs to import the Buffer type used in its API.

Third, the nullable dependency was added since using toMaybe is simpler than passing in the Nothing and Just 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:

  • 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)

- 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`
@JordanMartinez JordanMartinez added the type: breaking change A change that requires a major version bump. label Jun 23, 2023
@JordanMartinez
Copy link
Contributor Author

@thomashoneyman This is probably good for an initial review.

Things to consider:

  • what tests should be added for the stuff I added here, if any?
  • Should more of the API I added be added to the MonadBuffer class?
  • It's kind of hard to verify that I've added all APIs because they are spread out across 3 modules: Buffer, ImmutableBuffer, and Encoding. It's not clear to me why this was done this way, nor whether it should be kept that way.

@JordanMartinez
Copy link
Contributor Author

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.

@thomashoneyman
Copy link
Contributor

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?

@JordanMartinez
Copy link
Contributor Author

If one is using one of the data constructors from Encoding that I removed, they would need to update the removed ctor used to the one it aliases. Similarly, if they are any places doing an exhaustive pattern match without a wildcard for encoding, it would need to account for the new ctor, Base64Url.

@JordanMartinez
Copy link
Contributor Author

Anupam looked at this code and suggested not inverting the module dependency for the MonadBuffer class. Instead, to deal with the type inference issue for this problem, we could export the implementations used in the MonadBuffer instances. The breakage here would be that Buffer.create's type signature would change:

-create :: forall m b. MonadBuffer b m => m b
+create :: Effect Buffer

In other words, Node.Buffer would stop reexporting the members from MonadBuffer and instead export its own monomorphic functions that are used to implement Buffer instance for that type class.

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).

@JordanMartinez
Copy link
Contributor Author

@thomashoneyman See #52

@JordanMartinez
Copy link
Contributor Author

Superceded by #52, #53, #54, #55 (and a future PR for the Encoding change, which may be undesired).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking change A change that requires a major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Effect.Uncurried for FFI
2 participants