Opening Webapi.Dom shadows `None` about bs-webapi-incubator HOT 17 CLOSED

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.

from bs-webapi-incubator.

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...

from bs-webapi-incubator.

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

from bs-webapi-incubator.

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)

from bs-webapi-incubator.

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

What about namespasing it more? E.g.

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

from bs-webapi-incubator.

I've actually proposed that myself here: #27 (comment)

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).

from bs-webapi-incubator.

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

module Foo = {
  type t = ...;

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

  ...
};

from bs-webapi-incubator.

I made a PR to just wrap all the types here: #162 not sure if that is a to radical change. But there is a risk that other things than None conflicts as well.

from bs-webapi-incubator.

Here is a less radical PR just wrapping the EventPhase in a module. #163

from bs-webapi-incubator.

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".

from bs-webapi-incubator.

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.

from bs-webapi-incubator.

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.

from bs-webapi-incubator.

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.

from bs-webapi-incubator.

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

from bs-webapi-incubator.

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.

from bs-webapi-incubator.

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.

from bs-webapi-incubator.

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

from bs-webapi-incubator.