Make `Fs.Constants` abstract? about reason-nodejs HOT 6 CLOSED

Yeah, you're totally right. I think that's the right call. I want to say we inspected the values at runtime to see what they were under the hood, but it's probably irrelevant. Abstract types make sense.

from reason-nodejs.

Thanks Austin. I just realized that these constants are expected to be used as ints, e.g. from https://nodejs.org/dist/latest-v10.x/docs/api/fs.html#fs_fs_copyfile_src_dest_flags_callback:

flags is an optional integer that specifies the behavior of the copy operation. It is possible to create a mask consisting of the bitwise OR of two or more values (e.g. fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE).

So the type of all flags would also need to be changed to Constants.t and also a logical-or operator (or function) would need to be added to allow doing bitmasks. But that is reasonably easy, e.g.

module Constants: {
  type t;
  ...
  let (||): (t, t) => t;
} = {
  type t = int;
  ...
  let (||)(t1, t2) = t1 lor t2;
};

from reason-nodejs.

On the other hand, if the constants are each instances of separate abstract types, then it's tricky to pattern match on them. And I can imagine use cases for pattern matching. What do you think?

from reason-nodejs.

Yeah you're right, that can also be solved by using a pri int type, since the docs do implicitly say that it should be an int.

from reason-nodejs.

from reason-nodejs.

Sure, I'll send a PR soon.

from reason-nodejs.