sikanhe / reason-nodejs

Node bindings for Reason and Bucklescript
MIT License
100 stars 18 forks source link

Fix readFile bindings #32

Closed tjdett closed 4 years ago

tjdett commented 4 years ago

Neither FileHandle.readFile or FileHandle.readFileWith worked, as their definitions were incorrect.

Tests have been added to verify expected behaviour.

TheSpyder commented 4 years ago

Note that FileHandle.read won't work either, it takes an options object not a series of parameters: https://nodejs.org/api/fs.html#fs_filehandle_read_options

However we only needed to fix readFile for our use case and we might end up down a rabbit hole trying to fix more than we need.

sikanhe commented 4 years ago

Doesn't readFile take an optional object? https://nodejs.org/api/fs.html#fs_filehandle_readfile_options

sikanhe commented 4 years ago

@TheSpyder We will fix those asap

TheSpyder commented 4 years ago

Doesn't readFile take an optional object? https://nodejs.org/api/fs.html#fs_filehandle_readfile_options

Yes, but it doesn't need to. It's <Object> | <string>, and later the docs say

If options is a string, then it specifies the encoding.

sikanhe commented 4 years ago

The whole nodejs api design is just nuts 😅