-
Notifications
You must be signed in to change notification settings - Fork 30
[WIP] Rescript v11 #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Rescript v11 #135
Conversation
cc @glennsl, do you have any thoughts on the changes in here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Js.Dict.t
assumes keys are sting
s, but they can also be Symbol
s, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😀🌲
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