Compiler: use globalThis, drop joo_global_object

[hhugo]

fix #699

• implement compatibility for the old joo_global_object

[hhugo]

cc @copy, @jchavarri, @TyOverby
Anyone interested reviewing and testing this ?

[TyOverby]

I can try to test this on the Jane Street codebase next week

[hhugo]

Tests pass on node 10 (which doesn't have support for globalThis) and node 14 and 16.
I was able to run test referencing the old joo_global_object.

@TyOverby, I'd like to make a new release soon including this change. Please let me know once you've run your testing

[hhugo]

It also fixed the issue reported by @jchavarri in #699 (comment)

[jchavarri]

Thank you!

I have more questions than anything else, but otherwise looks great to me.

[hhugo]

Thank you!

I have more questions than anything else, but otherwise looks great to me.

Thanks for the review, can you close threads once you're happy with the answer

[jchavarri]

@hhugo I don't think I can resolve conversations as I didn't open the pull request nor have write access to the repo.

[TyOverby]

@hhugo I got this branch building internally, but we have so much javascript that uses "joo_global_object" that it'll take a long time for me to make the necessary changes in order to tell if there are any other issues.

Also, I must admit that this feature confuses me a bit; I would have expected that you'd use globalThis to generate wrapping-functions that look like this:

(function (joo_global_object) {
   // ... code goes here ...
})(globalThis)

Where we still keep joo_global_object, but the way that the object is acquired is by looking at globalThis.

Instead, it appears that the function-parameter name is also changed, which requires all of the rest of the changes in this feature.

[TyOverby]

Also, what does "implement compatibility for the old joo_global_object" refer to?

[TyOverby]

Here's how I assumed that the feature would be implemented: #1195

[hhugo]

@TyOverby , it is better to read this PR commit by commit.

The first commit just changes how we compute the global object (using globalThis), similar to your #1195.

The second commit cleans up the old joo_global_object name, renaming it to globalThis. We don't need a Jsoo-specific name, just use what the JS ecosystem uses.

The third commit make the stubs using the old joo_global_object work again (by introducing avar joo_global_object = globalThis statement if needed).

[hhugo]

but we have so much javascript that uses "joo_global_object" that it'll take a long time for me to make the necessary changes in order to tell if there are any other issues.

I don't understand. You don't need to change anything, it should just work out of the box

[TyOverby]

I don't understand. You don't need to change anything, it should just work out of the box

Sadly something appears to be broken as every test in the tree fails with the message "undefined variable joo_global_object"

[TyOverby]

The second commit cleans up the old joo_global_object name, renaming it to globalThis. We don't need a Jsoo-specific name, just use what the JS ecosystem uses.

What is the benefit of this approach?

[TyOverby]

I don't understand. You don't need to change anything, it should just work out of the box

Sadly something appears to be broken as every test in the tree fails with the message "undefined variable joo_global_object"

I won't be able to investigate this further for a few days due to the holidays, but I'll dig deeper on Monday

[TyOverby]

the failure happens here:

https://github.com/janestreet/core/blob/master/core/src/strftime.js#L43

if(joo_global_object)joo_global_object.strftime = adaptedStrftime}
   ^
ReferenceError: joo_global_object is not defined

[TyOverby]

Fixing that instance just causes the same error to happen in other places that we use joo_global_object. Is there a flag or something that we need to turn on in order to get the "it just works" shim?

[TyOverby]

introducing avar joo_global_object = globalThis statement if needed

I can confirm that some sections get this added to the top of them, but not all. Of note, at least two sections use joo_global_object but don't get the shim.

[hhugo]

introducing avar joo_global_object = globalThis statement if needed

I can confirm that some sections get this added to the top of them, but not all. Of note, at least two sections use joo_global_object but don't get the shim.

Thanks for testing, I have a theory that I should be able to test/fix early next year

[hhugo]

@TyOverby, I think I've fixed your issues in my last commit.
core_kernel/src/runtime.js and virtual_dom/src/hooks.js are using joo_global_object but were not getting the shim.

(Also note that core_kernel declare strftime.js but doesn't seem to use it)

[TyOverby]

@TyOverby, I think I've fixed your issues in my last commit.

Yep! I can confirm that the issue is no longer present!

Sadly it made another problem visible; I don't know if it's directly caused by this pull-request, but I'm seeing some strange behavior. A single javascript file (it's a 3rd party js library that we pull in) is being split between IIFEs like so:

runtime.js

var x = (function () { ...})();
if (typeof window !== 'undefined') window.x = x;
if (typeof globalThis !== 'undefined') globalThis.x = x;

Goes through Js_of_ocaml and out comes this:

output.bc.js

(function(globalThis) {
    var x = (function () { ...})();
}(globalThis));

(function(globalThis) {
    if (typeof window !== 'undefined') window.x = x;
    if (typeof globalThis !== 'undefined') globalThis.x = x;
}(globalThis));

Because the definition of x out of scope from the place that uses it, the 2nd IIFE will crash.

[TyOverby]

I spent a few hours trying to get a minimal repro but could not.
I'll be on vacation for the next two weeks, but would be happy to continue my investigation when I get back.

[TyOverby]

It looks like I'm able to work around that specific issue by wrapping the whole file in another IIFE.

[hhugo]

@TyOverby, I've managed to reproduce your issue with the following

var x = (function () { ...})();
//Always .. a comment starting by //Always
if (typeof window !== 'undefined') window.x = x;
if (typeof globalThis !== 'undefined') globalThis.x = x;

The behavior was introduced by c6d8608
and should now be fixed on master by 3e7f728

I'll rebase this PR to make it easier to test for you

[hhugo]

@TyOverby, I'm going to merge now, please make sure master works for you when you return from vacation.

[TyOverby]

@hhugo all of our automated tests are passing; we're going to do some more manual testing next week.

Thanks for all the help!