[WIP] Rescript v11#135
Conversation
|
cc @glennsl, do you have any thoughts on the changes in here? |
glennsl
left a comment
There was a problem hiding this comment.
With one exception, I think this looks very good!
| | @as(null) Null | ||
| | String(string) | ||
| | Number(float) | ||
| | Object(Js.Dict.t<t>) |
There was a problem hiding this comment.
Js.Dict.t assumes keys are stings, but they can also be Symbols, according to the spec, and in practice any other primitive type as well. Although I'm not sure the latter is a significant difference since primitive values automatically coerce to and from string.
There was a problem hiding this comment.
Yeah, that's a good point. Currently you'll need to take care of stringifying the keys yourself.
| // We're intentionally only bringing the constructors of `nullable` into scope | ||
| // here. Reason is if we do the same for `null`, we'll get issues with inference | ||
| // in the global scope, and we prioritize nullable higher because that covers | ||
| // more cases. | ||
| @unboxed | ||
| type nullable<+'a> = Js.nullable<'a> = Value('a) | @as(null) Null | @as(undefined) Undefined |
There was a problem hiding this comment.
Good choice! It would have been even better for usability if this was structurally typed, I think, but I assume there are good reasons for using plain variants instead. The discussion would also be out-of-scope for this repo anyway. Given that it is nominally typed, giving priority to nullable seems like the optimal decision.
| @@ -1,4 +1,4 @@ | ||
| type t<'a> = Js.Null.t<'a> | ||
| @unboxed type t<'a> = Js.Null.t<'a> = Value('a) | @as(null) Null |
There was a problem hiding this comment.
Nice. I like this better than the christmas presents 😁 Although null is definitely also a value, it's hard to think of a name that makes the distinction accurately.
There was a problem hiding this comment.
😀🌲
Yeah the naming wasn't easy. We chose between this and Present, and I personally think Value is slightly clearer, although I do agree with your point.
|
@glennsl thank you for the feedback! We have a bunch of helpers in and around the affected modules. I'm thinking we can go 3 routes here.
What are your thoughts on that? |
|
It's a bit hard to foresee how this is going to be used in practice I think. I can see people thinking this should just replace I wish I could offer something more helpful, sorry 😐 |
|
Good points. I lean towards that too - while this is convenient for interop, it's more worth focusing our efforts on Thank you for your thoughts! |
|
Current plan is to soon merge and publish this under Core |
|
Superseded by #189 |
Closes #132