Opening Webapi.Dom shadows `None`

[cassiozen]

Reason beginner here, so apologies in advance if this is not a bug.

If I open Webapi.Dom, it shadows the None constructor, and I cannot have optional variables in the same file.

It currently shadows None with Webapi.Dom.eventPhase.

[glennsl]

Hmm, seems to work fine here: https://github.com/reasonml-community/bs-webapi-incubator/blob/master/examples/dom_example.re

It's generally recommended to open modules as locally as possible, unless they've been explicitly designed for the purpose, precisely to avoid name collisions like this. It's still not a great idea to have a variant named None however, and I'll probably change it in the future. I'll keep this issue open as a reminder. Thanks for making me aware, and let me know if it's still blocking you.

[cassiozen]

Thanks for the response - I just got it to work.

This only happens when I rely on type inference:

If I type my variable as option(int) than None is correctly identified...

[AriaFallah]

I'm getting this as well (I am able to fix it with a local open, thanks @glennsl):

  Warning number 45
  MyFile.re 3:1-15

  1 │ open Types;
  2 │
  3 │ open Webapi.Dom;
  4 │
  5 │ open Webapi.Canvas;

  this open statement shadows the constructor None (which is later used)

  We've found a bug for you!
  MyFile.re 26:51-59

  24 │ };
  25 │
  26 │ let interval: ref(option(Js.Global.intervalId)) = ref(None);
  27 │
  28 │ let create = (config: config, ctx: Canvas2d.t) : t => {

  This has type:
    ref(Webapi.Dom.eventPhase)
  But somewhere wanted:
    ref(option(Js.Global.intervalId))

  The incompatible parts:
    Webapi.Dom.eventPhase (defined as DomTypesRe.eventPhase)
    vs
    option(Js.Global.intervalId)

[alex35mil]

In my case explicit type doesn't help, None is still shadowed.

What about namespasing it more? E.g.

module EventPhase = {
  type t =
    | ...
    | None;
};

[glennsl]

I've actually proposed that myself here: https://github.com/reasonml-community/bs-webapi-incubator/issues/27#issuecomment-282928267

Another alternative is to use modules with constants instead of variants, which dead code eliminate well and works really well with bitmask flags, but loses out on exhaustiveness checking.

A third alternative is to use polymorphic variants, which doesn't need wrapping in modules and can potentially be eliminated completely at compile time (unfortunately I think bs.unwrap currently is unsound).

[alex35mil]

Yeah, I like module wrapper pattern. Started using this recently and it feels very convenient:

module Foo = {
  type t = ...;

  let fromString = ...;
  let toString = ...;

  ...
};

[spocke]

I made a PR to just wrap all the types here: https://github.com/reasonml-community/bs-webapi-incubator/pull/162 not sure if that is a to radical change. But there is a risk that other things than None conflicts as well.

[spocke]

Here is a less radical PR just wrapping the EventPhase in a module. https://github.com/reasonml-community/bs-webapi-incubator/pull/163

[glennsl]

THanks @spocke. But I think if we're going to break things, we should do it properly to not break things more than strictly necessary. And for a change as invasive as this I think we need to make an experimental branch and try it out for a bit in real projects.

What are the options we have? From reading this thread I see:

1. Not including the Types module in Webapi.Dom This is basically the same as namespacing the entire Types module. This resolves the shadowing/name collision issue, but not much else.

2. Namespacing each type individually, by wrapping each of them in a sub-module. Also just resolves the shadowing/name collision issue, but is significantly nicer than a single Types namespace.

3. Using constants instead of variants, wrapped in separate sub-modules. Dead-code eliminates really well, works consistently with bitmask flags but loses exhaustiveness checking.

4. Using polymorphic variants with bs.as. If this works now, it's potentially zero-cost and maintains exhaustiveness checking, but does not work as well with bitmask flags.

For me it's between 3 and 4. The questions I have are, how useful is exhaustiveness checking for these types, and how important is it to be "idiomatic"? I'm leaning towards "not very much" and "not so much at this low level".

[spocke]

I would go with 2 since I think it's strange that these things are root level. For example nodeType is something I would expect to be in WebApi.Dom.Node.nodeType together with the decoderNodeType or in a separate WebApi.Dom.NodeType module same with compareHow it's only used by Range so in my mind that should be WebApi.Range.compareHow together with it's encoder the same probably goes for most of those things moving them to their appropriate module or in a common shared module with the same concept if they are using in multiple places but maybe I'm missing some problem with doing it that way.

[glennsl]

3 is namespaced as well, and 4 is structural, so it doesn't really matter whether it's at root level or not. What's the reason you prefer 2 over these?

If we put the types in the modules where they seem to most belong, we'll soon encounter issues with cyclic dependencies and will have to move at least some of them to a common module to resolve that. It therefore seemed more consistent to do so with all of them from the start. But we can still alias nodeType in Dom.Node, for example.

[yawaramin]

I'd say let's revisit a larger redesign in a separate issue. It is possible to mitigate the original issue reported here, the shadowing, by re-exporting the option type in some module before opening the Webapi.Dom module. For example,

module Option = {
  type t('a) = option('a) = None | Some('a);
};
...
open Webapi.Dom; // Although we don't recommend global opens
...
let none = Option.None; // No more shadowing error

If you don't global-open Webapi.Dom, then you can access the event phase with Webapi.Dom.None.

[TheSpyder]

I disagree. This library is cumbersome to use without open Webapi.Dom at the top of the file, as shown in the example code.

[yawaramin]

The example code doesn't necessarily reflect real-world code. If you're doing a lot of DOM-heavy work, sure, it makes sense to open it in a larger scope. But chances are, most people are not (otherwise we'd see a lot more interest in this issue, for example). In any case, a mitigation for None has been provided.

[TheSpyder]

I am doing a lot of DOM-heavy work with this library, sometimes I wonder if I'm the only one, your workaround (using Option.None instead of None) is not acceptable to apply across an entire codebase.

Ever since @spocke made his comments and PR proposals last year we've been using our own custom-named duplication of Webapi__Dom.re that replaces include Webapi__Dom__Types; with module Types = Webapi__Dom__Types;. I guess we'll be continuing to use that for the foreseeable future.

[yawaramin]

Hmm, I have an idea that's kind of a middle ground. Reopening this one.

[yawaramin]

None constructor is namespaced, will remove the old type in a few months