RescriptReactRouter crash on master (window vs. $$window)

[cknitt]

On current master, RescriptReactRouter.useUrl() crashes.

The actual crash is in the first line of RescriptReactRouter.hash():

function hash() {
  let window = typeof window === "undefined" ? undefined : window;
  if (window === undefined) {
    return "";
  }
  let raw = window.location.hash;
  switch (raw) {
    case "" :
    case "#" :
        return "";
    default:
      return Js_string.sliceToEnd(1, raw);
  }
}

The error I get in the browser is

ReferenceError: Cannot access 'window2' before initialization

On the ReScript side we have

  switch %external(window) {
    ...

AFAIK %external is used to guard the access of window with typeof so that the code won't crash if run on a platform that doesn't have window.

Before #6831 the first line of the JS output was

  var $$window = typeof window === "undefined" ? undefined : window;

so the problem did not occur.

/cc @cometkim @cristianoc

[cknitt]

The undocumented extension %external is used in rescript-react in quite a few places to access the globals window and history in a safe way.

[cometkim]

The error I get in the browser is

ReferenceError: Cannot access 'window2' before initialization

Where exactly does the ReferenceError come from? I expect it will be a SyntaxError of re-declaration

Ok it will be a ReferenceError in a function context for sure

---

And the existing %external behavior doesn't feel right to me. If it were to reflect the semantic of keyword "external", it should create an explicit reference to globalThis instead of blocking the using variable name window.

So it could be

let window = typeof globalThis.window === "undefined" ? undefined : globalThis.window;

// and it could be better
let window = globalThis.window; // treat as an option

[cometkim]

I could fix the builtin %external PPX behavior for compatibility.

but I think it's better to deprecate (at least rename it) it in the end. Just having Js.globalThis should be enough IMHO. Actually, I've already suggested something related in the past.

https://forum.rescript-lang.org/t/is-it-good-idea-having-binding-to-js-this/2922

[cometkim]

Umm... found a counterexample from test.

https://github.com/rescript-lang/rescript-compiler/blob/1d46abe3/jscomp/test/undef_regression2_test.res#L21

__DEV__ is clearly expected to be replaced by the bundler's define plugin, not an actual reference.

[cknitt]

globalThis is actually a very good point. In rescript-react, we can just have

@scope("globalThis")
external window: option<Dom.window> = "window"

@scope("globalThis")
external history: option<Dom.history> = "history"

[cometkim]

Yep. so I'm now trying to split it into %external (alias to %global) and %define. And the %global should be replaced by the globalThis reference.

[cknitt]

Yep. so I'm now trying to split it into %external (alias to %global) and %define. And the %global should be replaced by the globalThis reference.

Maybe we should better get rid of this undocumented feature altogether and replace it by bindings or %raw where needed.

There does not seem to be too much usage across the ecosystem: https://github.com/search?q=%22%25external%22+language%3Arescript&type=code

[cometkim]

That's something to decide. I can agree that it's a convenient sugar syntax to have. It's also good to have for compatibility reasons.

The reason it is not used not much is because the API has not been officially introduced.
(That's fortunate. The existing %external is a footgun)

We can get rid of it, or make it more sensible and introduce as an officially supported feature.