Mapping ocaml unix APIs to nodejs calls

[phated]

This is just a draft right now because I need a lot of advice on TODOs and testing (sorry, first time contributing here).

@hhugo saw my work on these utilities in the Grain repository and said I should look into upstreaming them. So here is my first pass.

I tried changing some of our assumptions to allow for non-nodejs usage, but I don't know what the functions should do if not inside nodejs.

Please help guide me into taking this over the finish line! 🍻

[phated]

If I'm looking at this correctly, it seems the only way to write tests that run in nodejs is to include them inside the compiler/tests-compiler/ directory and use Util.compile_and_run - is this correct?

[phated]

@hhugo if you have some time to look over this, I'm hoping to get this over the finish line within the next week-or-so!

[TyOverby]

If I'm looking at this correctly, it seems the only way to write tests that run in nodejs is to include them inside the compiler/tests-compiler/ directory and use Util.compile_and_run - is this correct?

Yes, this is correct. I’m not sure if any of the runtime js files are linked by default though, you may need to make changes in Util

[phated]

Yes, this is correct. I’m not sure if any of the runtime js files are linked by default though, you may need to make changes in Util

Hmm, I'm not sure what you mean

[phated]

I'm not sure what else to do here without guidance (specifically "you may need to make changes in Util") so I'm marking this ready-for-review to hopefully get some attention

[hhugo]

Yes, this is correct. I’m not sure if any of the runtime js files are linked by default though, you may need to make changes in Util

Hmm, I'm not sure what you mean

That mean Util.compile_and_run might not give enough flags when calling the js_of_ocam compiler, one might need to tune util.ml to do so.

I don't think you need to worry about this as unix.js is already part of the default runtime.

[hhugo]

If I'm looking at this correctly, it seems the only way to write tests that run in nodejs is to include them inside the compiler/tests-compiler/ directory and use Util.compile_and_run - is this correct?

This is not the only way. Look at tests in lib/test (e.g. https://github.com/ocsigen/js_of_ocaml/blob/master/lib/tests/test_nodejs_filesystem_errors.ml). Theses tests run in node only, the dune file says (inline_tests (modes js))

[hhugo]

I though more about this. I would prefer to integrate with the "js_of_ocaml filesystem proxy" with something similar to what we have with caml_sys_rename

function unix_stat(n){
  var root = resolve_fs_device(n);
  if(!root.device.stat)
    caml_failwith("unix_stat: no implemented");
  root.device.stat(root.rest);
}

You would then have update fs_node.js with new methods

[hhugo]

Improves #483

[phated]

I though more about this. I would prefer to integrate with the "js_of_ocaml filesystem proxy"

Excellent! Happy to do this. You are okay with JS built for the browser failing with "unix_stat: not implemented"?

[hhugo]

I though more about this. I would prefer to integrate with the "js_of_ocaml filesystem proxy"

Excellent! Happy to do this. You are okay with JS built for the browser failing with "unix_stat: not implemented"?

That or we implement some dummy stat.

[phated]

That or we implement some dummy stat.

One thing I considered for Grain was embedding the file stats of the built JS file for anything in memory, since any embedded files would be a snapshot at that timestamp. Is this a good idea? If so, any thoughts on how I'd implement that?

[hhugo]

Can you describe the grain use case a bit more ?

[phated]

Can you describe the grain use case a bit more ?

Currently, Grain compiles the compiler (written in Reason) to JS using JSOO and then includes it into a nodejs runtime called pkg. pkg provides the filesystem abstraction for us as long as the resulting JS calls nodejs APIs that we expect (the current implementation of this logic). Eventually the primary compiler will move to native executables.

However, I also did some research into a browser bundle to add a playground on our website—this should be a natural workflow, as we can compile to WebAssembly in the browser, and then directly run it in a WebAssembly context. Unfortunately, we'd need the unix methods supported in the browser because our compiler compares timestamps of files for caching and recompilation. I was planning to add this feature in the future when work began on our in-browser editor.

[hhugo]

The current implementation of fs_fake.js is a bit naive currently. With some work you should be able to add additional information like stat.

[hhugo]

I don't have more to say right now, but I'm happy to answer any question if you spend some time working on it.

[phated]

Thanks! For this first draft, I'll not handle the browser APIs, but I will plan to revisit in the future.

[phated]

@hhugo this should be ready for review! I added some tests that I could think of for the Unix calls that were implemented. Let me know if you think there should be more tests.

[hhugo]

It took me a while get the full picture of changes. Some comments probably reflect that.
The one thing I don't like about it is the raise-catch-raise business. see my comment about adding an extra argument.

[phated]

@hhugo Thanks for the feedback! I've updated the MlNodeDevice methods to take the raise_unix flag as the last argument and use that to determine if Sys_error or Unix_error exceptions are raised.

There are a couple decisions to be made still (unresolved comments) but I'm happy to get those fixed up in the morning.

[phated]

Can you rebase your PR.

Done!

[hhugo]

One more thing, could you add tests for the new functions but under the /static/directory. The tests should show that some features are not implemented yet. It will be useful if you/one decide to add the support.

I few more tests and we should be ready to merge

[phated]

few more tests and we should be ready to merge

I'll try to get to that this afternoon. Have a couple of job interviews, but hope to have time outside of them.

[phated]

I added the tests in the same way you added Unix.mkdir and Unix.rmdir tests, but I added the Failure messages to the expected output. If there is a better way to handle these, please let me know 🙏

[hhugo]

Thanks a lot for the hard work, I will merge once the CI is happy.