sikanhe / reason-nodejs Goto Github PK

The ~write callback for Stream.Writable.makeOptionsObjMode has the wrong number of parameters. Even though encoding is ignored for objectMode, it is still required to be provided or else the callback parameter does not make it through.

From what I can tell, this issue extends to most callbacks for objectMode.

We need more tests. This issue exists to clarify the testing goals of the project and give future contributors an idea of what we want to see in the test suite.

Goals:

1. Ensure the type bindings are mostly accurate and sound.
2. Reduce the likelihood for bugs, both at compile-time and at runtime.
3. Make sure functions are called/applied correctly.
4. Increase the overall quality of the library.
5. Keep track of breaking changes (once this library is officially stable).

Non-goals:

1. 100% type correctness -- Node's API was designed many years ago with JS developer ergonomics in mind. Therefore, many of its functions are highly polymorphic and dynamic when it comes to types. It is not possible to perfectly model everything in Reason's type system, and we have no desire to do so.
2. 100% API coverage -- Some things are hard to test. Some things aren't worth testing due to sheer complexity, such as isolating test code from external factors. Much of Node is focused on IO operations, which are notoriously difficult to test. We won't sweat over it. Nor should you.
3. Test the underlying functionality of Node -- Node has their own internally-maintained test suite that they use for development. They got it covered.

With those ideas in mind, we invite anyone and everyone to contribute to the test suite. Just submit a PR and we will be happy to integrate it if it meets our testing goals.

This is also a great way for Reason beginners to contribute to the ecosystem!

Fs.createWriteStreamOptions should return something of createWriteStreamOptions instead of createReadStreamOptions

In ChildProcess, execSync is spec'd to return a string. However, it will always return a Buffer.t. execSyncWith can return a String if an ~encoding is specified like "utf8" . Otherwise, it defaults to a Buffer.t

For confirmation, see the types repo

It seems to me, from the way readFileSync binding is done, I cannot provide "utf8" option yet.
Could someone confirm it?

readFileOptions defined as,
https://github.com/sikanhe/reason-nodejs/blob/master/src/Fs.re#L203-L204

Flag defined as,
https://github.com/sikanhe/reason-nodejs/blob/master/src/Fs.re#L140-L189

I need to give utf8 to read text files properly as in NodeJs example:
https://nodejs.dev/learn/reading-files-with-nodejs

Looks to me, I should add something like the following to the definition of Flag.

...
  [@bs.inline "utf8"]
  let utf8: t;
...
  [@bs.inline "utf8"]
  let utf8 = "utf8";
...

But I am not 100 % sure (because I am Reason beginner still). Should I make PR with the above code change or could the library maintainers add utf8 flag for readFileSync function?

Thank you :)

It's important for this library to be consistent across modules in things like naming conventions, binding optional parameters, handling options objects, using named arguments, etc.

We also use a couple of techniques globally that ought to be used everywhere they apply (in particular, Stream sub-typing, use of BinaryLike.t('a), etc).

With consistency and quality in mind, I am posting this set of guidelines to follow when we run into situations where multiple possible solutions exist. That way we can settle on exactly one of those solutions and stick to it.

Principles

• In general, we do not wrap external bindings with our own functions. This can make it tricky to write ergonomic bindings sometimes, but the benefits are worth it.
• We want to model the types perfectly, but sometimes that is not practical. In those cases, it is better to bind to a simplified (yet still valid) version of the type signature. Additional bindings can be written later if we need to.

Naming & Techniques

• IF an API function specifies any number of optional parameters

  • THEN the binding should be split into two functions. The first one will have the original name from the API, and will not include any of the optional parameters. The second function will be named <original name>With (e.g. execWith), and will specify all of the optional params as 'optional named parameters'.
  • EXCEPT in certain edge cases, like when a function's output type depends on one or more of its optional arguments. In that case, we do whatever is sensible.

• IF an API function specifies a options object parameter

  • THEN we create an abstract type for that object, named <function name>Options, and then write an external function to construct that object using the [@bs.obj] attribute. This allows us to specify the object type in the function signature and give users a convenient way to construct a partial version of that object using optional named arguments in the object constructor, which usually matches the API's intent.
  • EXCEPT when the options object is also a return value in part of the API, in which case the type cannot be abstract. In that case, the only difference should be to make the type concrete as a record type. It's also a good idea to use [@bs.as] on the property names, to guarantee they won't ever get mangled by the compiler. The function signature should go unchanged.

More will be added later. This is all open to discussion and critique, so feel free to offer any opinions or concerns in the comments. If we reach an agreement on something, I will edit this post to reflect any changes.

In the file https://github.com/sikanhe/reason-node/blob/master/src/Fs.re , the file permissions flag is repeated for several bindings:

https://github.com/sikanhe/reason-node/blob/4e3243fa321373a8f023b5b9689e569a07b2bd5a/src/Fs.re#L56

https://github.com/sikanhe/reason-node/blob/4e3243fa321373a8f023b5b9689e569a07b2bd5a/src/Fs.re#L79

Etc.

We can factor out a flag type and inline the values, using the technique shown here: https://dev.to/yawaramin/inlined-values-in-bucklescript-247c

In fact the same thing can be done for the encoding parameter which is also repeated several times.

BS 8.2 changed how @bs.as works, so now it doesn’t compile some of the [@bs.as {json|...|json}] in reason-nodejs.

It appears Process and ChildProcess modules' stdin, stdout, and stderr values are all the opposite type from what they should be.

Maybe the Fs promise API is not so desirable for everyone, i.e. using it with Relude.IO is unnecessarily complicated; having to ignore, then_ and catch the promise values. Another drawback of using Js.Promise.t is the opaque error type where the callback versions actually have the error type well defined (mostly the built-in Error object with at least message and name I think).

Regarding Streams (bound in ReasonNode.Stream) and them being instances of the EventEmitter class:
How would you handle (emit and on) for custom events?
There are several parser (and other libs) in the node eco-system leveraging custom events. How would you bind those libraries using ReasonNode.Stream? Separate custom binding of on for each?

I understand it's complicated (if even possible) to find a generic solution due to the polymorphism of the EventEmitter class. I'd like to know your thoughts and ideas on this topic.

Thank you both for your huge efforts and amazing work!

Can't figure out how to do this.
Am I missing it?

I focused on the Stream bindings, but this probably holds true for other parts of this repo as well.

Currently the return type of a callback is unit. Therefore it's possible to just return any expression returning unit. To prevent missing callbacks, they could be typed to return an abstract type. The function passing in the callback would then expect this abstract type.
This way the compiler would know if you forgot to call the callback.

I used this approach in some adhoc bindings of streams and a parser and liked it. What are you thinking about this?

For newcomers like myself, it'd be nice for the examples to show that the library modules should be called with the NodeJS namespace, like:

NodeJs.Fs.access("foo")

The examples leave the namespace out.

While documentation is not the top priority, we do want to provide some basic documentation for commonly used functions. This is something we plan to work on gradually over the long term.

In the meantime, we welcome all contributions to documentation in the form of pull requests. If you see something that you want to add, please let us know and/or submit a PR.

This is also a great way for Reason beginners to contribute to the ecosystem, and possibly even learn a few things about Reason & Node.js.

Currently, the Node fs module constants are bound as int:

But in the Node fs docs, it's never specified that they are numbers. So I'm wondering if you would be open to making the constants bindings an abstract type, e.g.

module Constants = {
  type t;

  [@bs.module "fs"] [@bs.scope "constants"] external f_ok: t = "F_OK";
};

And so on. I can also add the rest of the constants in the module.