bs.js modules imported in commonjs format with "es6" config

[jchavarri]

Pretty minor, but when using "module": "es6" in bsconfig.json, the main BuckleScript file that is consumed from the .re.js files is still being imported using require:

// $FlowExpectedError: Reason checked type sufficiently
const DemoBS = require('./Demo.bs');

The expected behavior would be that these imports also comply with the module config value.

[cristianoc]

That's a good point, though doing that I get a couple of problems.
One of them is that the circular imports between .re.js and .bs.js, generated when importing JS values, break.

[jchavarri]

One of them is that the circular imports between .re.js and .bs.js, generated when importing JS values, break.

Why do they break? es6 modules definitely support circular imports by design (unfortunately).

If it's because of BuckleScript and genType having different configs, I think users should configure the project to use the same modules format for both tools.

Maybe genType could deprecate the gentypeconfig.module config and instead read the module format from BuckleScript's package-spec.module? (now that gentypeconfig lives in bsconfig.json). That way, there would be no chance of mixing es6 with commonjs.

[cristianoc]

I've tested with bs compiling to es6, so it was all es6, and got some webpack runtime error.
I had already noticed errors when mixing commonjs and es6 imports, but this one was entirely within es6.

It could be because
constant c = require('...')
leads to delayed imports. So moving to es6 actually changes the semantics.

So it's possible that playing with things a bit will give full support without errors.
The thing is, all the tools in the pipeline could possibly introduce some issues, and I don't have enough detailed information to know what you can and cannot trust.

[jchavarri]

Ah, I see. Modules in JS are unfortunately not ideal.

This is not a big deal in any case, I opened it because I was exploring how to bundle a genType exported lib with Rollup, and it can only do tree shaking with ES6 modules:

with CommonJS, the entire tool or library must be imported.

Having requires in .re.js files means that the resulting bundle would include all the values exported from the sibling bs.js file, even if only a few of them are annotated with @genType.

[cristianoc]

@jchavarri I've created a pull request to illustrate the issues I've encountered:
#74

It contains the generated files, and opening src/interop/index.html from examples/reason-react-example shows how there's a runtime error wrapJsValue.bs.js:7 Uncaught TypeError: .

WrapJsValue.bs.js:7 Uncaught TypeError: _WrapJsValue_re__WEBPACK_IMPORTED_MODULE_0__.round is not a function
    at eval (WrapJsValue.bs.js:7)
    at Module../src/interop/WrapJsValue.bs.js (interop.js:2782)
    at __webpack_require__ (interop.js:20)
    at eval (WrapJsValue.re.js:9)
    at Module../src/interop/WrapJsValue.re.js (interop.js:2794)
    at __webpack_require__ (interop.js:20)
    at eval (InteropRoot.js:24)
    at Object../src/interop/InteropRoot.js (interop.js:2723)
    at __webpack_require__ (interop.js:20)
    at interop.js:84

[cristianoc]

This error goes away after e.g. manually replacing the import with require in that file.
Hopefully there's a way to get around the issue. I've checked in the webpack output, so it should be easy to repro.

[jchavarri]

Hey @cristianoc I took a look at the PR and could repro. (I actually faced another issue where Webpack would read a file.re import and try to process the actual Reason .re file and broke... not sure what was going on).

I also found this issue where the Webpack creator clearly states this is the expected behavior, as the specs defines:
webpack/webpack#1788 (comment)

In the real world, when I've seen this problem, it was solved by manually delaying evaluation, so the module would export a function that, when called, would return the exports. I created #89 just to show how this approach could look with genType. Due to the issue with Webpack I mentioned above of processed .re files, you'll see I renamed the .re files to take out the dot:
https://github.com/cristianoc/genType/blob/c227f4dcba18282f76892725ee55f8874a44a22a/examples/reason-react-example/src/interop/InteropRoot.js#L3

I'm not sure if this is a Webpack bug, I'm confused about it... but once I renamed the files, things started working again.

I think in general, the circular dependencies problems are known in JavaScript world. This doesn't make them less painful, but I'm not sure that genType should take ownership and fix them (how?). One thing I wonder is if the decision of using both .bs.js and re.js files could potentially increase the chances that the problem arises when using genType...

[cristianoc]

@jchavarri thanks for looking into this.
I must admit that I don't understand everything above.
So I've updated #74 to be a minimal repro.
Only a couple of generated files, to be edited directly, and webpack to be run on them. No .re files even.

So the starting observation is that by changing the commented out line https://github.com/cristianoc/genType/pull/74/files#diff-8502f86b7087f06d482fab696fbb033f the issue does not occur anymore.

The other observation is that in the minimal repro, I believe all the code is ES6.
Also, there are no default exports.

[cristianoc]

So could we start again from the minimal repro instead?

[jchavarri]

What I meant above is that lazy exports work with ES6. I updated #89 with your branch to show an example (that code works correctly, but the exported values have to be wrapped in a function).

there are no default exports.

I don't think the problem is related to default or named exports, it's a problem with ES6 modules in general. As this SO answer mentions:

What's the best practice about circular dependencies in ES6?

Avoid them altogether. If you cannot (or don't want to) completely avoid them, restrict the involved modules to only use function declarations with circular dependencies, never initialise top-level values (constants, variables, classes) by using imported values (that includes extends references).

One way to get rid of this problem could be to consolidate .re.js files and bs.js files into the same file. Not sure if that's possible today though. 🤔

[cristianoc]

@jchavarri thanks for the update. The main difficulty is that at the moment the post-processing happens after bucklescript has run, so there's no way to affect the code which will go into the .bs.js file. This is also the reason why one cannot combine .re.js and .bs.js.

Since this import of .bs.js files from .re.js files only happens when using @genType.import, it should be possible to only emit require's for that specific case. Thought I don't know whether this would help.

[jchavarri]

Yeah, I think only emitting require imports in that case would be a good trade-off.

Also, I was researching a bit more and found this post that outlines a different idea, that involves having an internal.js file, where all the imports are accumulated and then exported, and then have all other files import from the internal: https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

I'm not sure if it's a good idea to have an extra added file as part of the genType artifacts 😅 Maybe this could be done only on special cases, so instead of emitting require when @genType.import is used, one could instead have this "buffer" internal.js file where all the imports are accumulated and then exported.

[cristianoc]

This quote from the post is definitely useful to set expectations:

"Secondly, whether this works is highly dependent on the module bundler. For example, in codesandbox, when bundling our app with Parcel (or Webpack or Rollup), this solution doesn’t work. However, when running this locally with Node.js and commonJS modules this workaround might work just fine."

So this clarifies that there are multiple implementations of circular imports in different systems. So any working solutions would need to be correct in all these implementations.