Object.get documentation and tests

[jmagaram]

I couldn't find the reference to this on MDN. I see all the static methods etc. but don't know where the property accessor is.

My tests revealed that getting a property with a null value returns Some(null).

The Object.get function returns a Some('a) which is kind of like a TypeScript any. I was able to get it to crash by unsafely treating this as an array when it is not. See the commented out test. Maybe you already knew about this? I didn't. I'm wondering if we need easier and safer ways of dealing with values of unknown type. I recommend adding a unknown type. And then we should mention this in the docs for this saying "Note: To safely convert Some(unknown) to a known type, use Unknown.classify or similar functions." And better, we should have Object.getString and Object.getFloat etc. that check the typeof and do a safe conversion there. See #102

I noticed there is a getSymbolUnsafe but not a getUnsafe. Why do we have one but not the other?

[glennsl]

Indeed, these functions are all very unsafe, both the one's that are actually marked unsafe and the one's that aren't. And they should all be marked as such. In general, whenever you have a type variable (actually a unification variable) on the output side that is not also on the input side, the function has an unsound type. The type will be inferred based on how it's used later, with no regard for the actual data type.

Additionally getSymbol and setSymbol are misleadingly named since they don't actually return or set a symbol. They returns and set a value by a symbol.

I'm otherwise not so sure about the Unknown module. I think the reason you'd use these unsafe functions is to avoid the overhead of doing it properly, and is sometimes useful for shims in bindings and such. Additionally, these are just the function equivalents of the obj["foo"] syntax, which would still be as unsafe. And of course there are many other escape hatches too, such as %raw, so I think this would mostly just introduce more inconsistency.

I strongly believe that these should be clearly marked as unsafe though, irregardless of what other conventions or rules would be broken by doing so. And that the docstrings should explain why they're unsafe. This is much more important than documenting and analyzing exceptions, since exceptions will just crash your app. Using these functions incorrectly will leave you in undefined territory, breaking the type system guarantees that we rely on for producing correct software, and creates the kind of bugs that will randomly drain you bank account.

[jmagaram]

Good catch on getSymbol. I will change the names to getBySymbol? Actually I see that getSymbol tons of places. Someone else should change it in a different PR.

Regarding Unknown module forget the name for a second. I want to access things safely. We have classify which takes us one step toward safety. But it is so awkward to use, especially compared to Javascript where you'd just slap a ["code"] onto the end. That's why I think it would be super helpful to have wrappers around classify like toString and toObject in that module to do this. Now it will still be safe but more convenient. Then add the toStringUnsafe variants if desired. And possibly indexed accessors. This is the kind of thing I want to prototype in an expanded Type/Unknown module. If Object.get returns an unknown you won't be able to use it unsafely.

let toString = i =>
  switch i->Type.Classify.classify {
  | String(s) => Some(s)
  | _ => None
  }

By the way in my code I'm just checking why an auth exception happened. I'll give a reasonable error message if the user name or password is invalid but probably re-throw most others.

[glennsl]

I'm with you on the usefulness of such a module, but once you get beyond simple primitives I think it would start to get quite opinionated, and I don't think that much opinion belongs in this library. I'd rather have a third-party library for it. There already are several for the very similar use case of JSON decoding, for example, like rescript-json-combinators (completely unbiased choice, of course, but I also think it has the simplest implementation and would be fairly easy to extend for JS types in general).

Good catch on getSymbol. I will change the names to getBySymbol? Actually I see that getSymbol tons of places. Someone else should change it in a different PR.

I'm in no position to make that decision, but yes, I think doing a separate PR to rename them across all modules is a good idea.

[jmagaram]

Good point about json parsing and not taking it too far or being opinionated. I hadn't thought about it in the larger context. I would like to see more help with primitives baked into the Types/Unknown module. That module has other problems like no bigint and using toString to classify. Should I take a stab at it? @glennsl @zth

[glennsl]

Again, I'm in no position to make that decision, but that implementation is indeed quite odd. It would make more sense to base it on the existing classification function that ships with the compiler I think.

[jmagaram]

Consolidated into #117