BS compiler: Output path awareness require statements

[thangngoc89]

Important links:

• Bucklescript's fork of for sketch: https://github.com/thangngoc89/bucklescript/tree/sketch
• Build instructions: https://github.com/Sketch-sh/sketch-sh/tree/598839aa67c4414b4439936457bbb6e723dbb026/engine_bs

A quick introductions about how compiler loads module and output require path

When BS compiler encountered a new module, it will look in the load path for Modulename.cmi and Modulename.cmj.

When outputing to javascript, BS compiler will output the require path as ./stdlib/modulename.js no matter where Modulename.cm(i|j) is located.

This becomes a problem for supporting external packages because we can't know if a module is part of BS's stdlib or an external package.

Proposal

Make the output path function awares of the Modulename.cm(i|j) path.

Example:

• workdir/stdlib/Belt_array.cmi -> ./bs/stdlib/belt_array.js
• workdir/reason-react/React.cmi -> ./bs/reason-react/react.js

Relevant code path: https://github.com/thangngoc89/bucklescript/blob/3614d992c55b71c2e253d7bc15a7420a6f4f7cd1/jscomp/core/js_name_of_module_id.ml#L203-L214

[jchavarri]

@thangngoc89 I am taking a look at this issue but I'm not sure I understand why the current BuckleScript implementation does not work.

If users want to use reason-react then it would get added to package.json and resolved through an external, like any other 3rd party library right? And in that case, BuckleScript resolves to simple require('reason-react') which would work fine.

What am I missing?

[thangngoc89]

Sorry for the lacks of information.

What you're describing would work if we compile external dependencies on the fly with bucklescript as well. That basically re-creating bsb (the build tool) which would be too complex and heavy for a client application.

So the approach I've chosen would be precompiled packages and grab the artifacts (cmi, cmj and js), put it on a CDN so that sketch could load these packages on demand.

At execution time, sketch's executioner would do this:

• Load cmi, cmj files into the compiler's virtual filesystem: /workdir/reason-react/ and add that directory into load path.
• Later, the compiler would references to those dependencies like this ./bs/reason-react/reasonReact.js, and sketch would know where to file this reasonReact.js file.

[jchavarri]

Thanks for explaining, that makes a lot of sense :)

So how would this new code in js_name_of_module_id.ml differentiate between stdlib modules and other modules? Should we whitelist all module names that are part of stdlib or is there another way that doesn't require this indirection? I'm not a fan of the stdlib whitelist idea as it could get out of sync, but can't think of any other approaches.

[thangngoc89]

So how would this new code in js_name_of_module_id.ml differentiate between stdlib modules and other modules?

By looking at where the .cmi is located.

/workdir/stdlib/belt_something.cmi is definitely stdlib

We can control the load path here, and if the cmi changed for some reason (aka a breaking change in bucklescript's upstream), we'll need to make a new engine release

[jchavarri]

So, from what I understand, this task would require splitting the | Runtime | Ml case which is currently handled similarly, into two different cases, right?

If this issue is for ReasonReact, it feels like that one should figure out how to load RR cmis/cmjs in the browser first, and then solve the way BuckleScript solves module ids?

In case it's useful, I checked how the current Reason playground does it. The PR was https://github.com/reasonml/reasonml.github.io/pull/332/ and the relevant code is in setupSomeArtifacts.

The core function is load_module that was added for that PR, and receives a cmi_path. So maybe if we use that we don't need to change js_name_of_module_id?

[thangngoc89]

this task would require splitting the

I don't know. I didn't read that part of code yet.

The core function is load_module that was added for that PR, and receives a cmi_path

It doesn't matter what's the cmi_path is, as long as we add that path into Config.loader here

What's this issue about is that bucklescript should output js require path relative to the cmi_path

Why do we really need to change js_name_of_module_id?

Because we need to be able to know which package we are referencing for loading the correct compiled js file.

If all require paths are under ./stdlib/ like this:

We can't support many packages because of: name conflicting, no way to keep track of current version.

[jchavarri]

Thanks for the explanation and the example @thangngoc89.

I am trying to get the cmj path from a loaded module using Lam_compile_env.get_package_path_from_cmj, which is what the native code is using, but I just get a constant string "BROWSER" for this path. It seems that cmj paths are not available in browser build of BuckleScript, this "BROWSER" constant gets added here.

This problem surfaced when loading modules through reason.compile_module function. Maybe this approach doesn't load path information?

I tried also using the reason-react branch in #237 which uses load_module but I get an error when trying to compile let a = React.string that says:

Error: The files /static/cmis/pervasives.cmi and /static/cmis/React.cmi bs.js:229:27 make inconsistent assumptions over interface Pervasives

Do you have any ideas? I already switched to opam switch 4.02.3 to make sure jsoo is in the right version. 🤔

[thangngoc89]

hmm, the make inconsistent error is weird. I probably messed up somehow

[jchavarri]

@thangngoc89 I just managed to build sketch locally with reason-react prebuilt, and still get the BROWSER string in the returned value from get_package_path_from_cmj.

I also tried using Config_util.find_opt js_file to see if that could work to get the path, but it always returns None.

So I'm not sure that the original way to achieve this task is possible with a "read only" approach. As far as I can tell, all the ways to get locations out of cmi/cmj files that work in native mode don't work in browser build of BuckleScript. It seems we should change the way modules are registered with load_module so we include the folder path too?

[thangngoc89]

@jchavarri

It seems we should change the way modules are registered with load_module so we include the folder path too

This is unrelated, the load_module and path generation aren't the same AFAIK. But whatever you can do to make it work, please do it.

Also, if something works differently between browser and node, you can search for this BS_COMPILER_IN_BROWSER to see the conditional compilation applied for browser.

Maybe we can ask @bobzhang ?

[bobzhang]

Also, if something works differently between browser and node, you can search for this BS_COMPILER_IN_BROWSER to see the conditional compilation applied for browser.

Yes, my original hack is just to assume they are in the same directory ./stdlib, I am open to make changes accordingly

[thangngoc89]

@bobzhang we're literally stucked here. So any pointers would be appreciated.

[bobzhang]

I will have a look this weekend

发自我的iPhone

------------------ Original ------------------ From: Khoa Nguyen [email protected] Date: Fri,Aug 2,2019 3:25 PM To: Sketch-sh/sketch-sh [email protected] Cc: Hongbo Zhang [email protected], Mention [email protected] Subject: Re: [Sketch-sh/sketch-sh] BS compiler: Output path awareness require statements (#229)

@bobzhang we're literally stucked here. So any pointers would be appreciated.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[thangngoc89]

@bobzhang thank you very much

[bobzhang]

@thangngoc89 see my comments in Sketch.sh #bucklescript channel

[jchavarri]

BuckleScript does not exist anymore and I don't think we will have the resources to build a brand new Sketch engine for ReScript anytime soon, so closing related issues.