Skip to content

[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

Closed
wants to merge 6 commits into from
Closed

[WIP] Rescript v11 #135

wants to merge 6 commits into from

Conversation

zth
Copy link
Collaborator

@zth zth commented Apr 27, 2023

Closes #132

@zth zth force-pushed the rescript-v11 branch from a2ef8e2 to 605507b Compare May 5, 2023 17:48
@zth
Copy link
Collaborator Author

zth commented May 5, 2023

cc @glennsl, do you have any thoughts on the changes in here?

Copy link
Contributor

@glennsl glennsl left a 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>)
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Comment on lines +67 to +73
// 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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@zth
Copy link
Collaborator Author

zth commented May 10, 2023

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

  1. Minimize the amount of helpers, removing most things that can now be done with pattern matching instead
  2. Leave it as is now with only slight adjustments as needed
  3. Extend the affected modules so that for example Null and Nullable have the same helpers available as Option, to the extent that's possible

What are your thoughts on that?

@glennsl
Copy link
Contributor

glennsl commented May 12, 2023

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 option, and it would then make sense that it offers the same functionality. But if I understand correctly there are subtle differences that will cause confusion if used as a direct replacement, such as with nesting and Value(Null) not being distinguishable from just Null. And so perhaps its use should be discouraged in favor of using option, which would point towards just treating them as raw values and minimizing the amount of helpers. I don't see a long-term case for just leaving it as-is, but short term it could make sense to see how things develop and reduce breakage in the meantime.

I wish I could offer something more helpful, sorry 😐

@zth
Copy link
Collaborator Author

zth commented May 12, 2023

Good points. I lean towards that too - while this is convenient for interop, it's more worth focusing our efforts on option (which after all is the most idiomatic thing). So just leaving as is and seeing how it's used is a good path forward, and then we can revisit when we have more real world data.

Thank you for your thoughts!

@zth
Copy link
Collaborator Author

zth commented Jul 14, 2023

Current plan is to soon merge and publish this under Core 1.x, and let current 0.x live on without full v11 support.

@zth
Copy link
Collaborator Author

zth commented Feb 7, 2024

Superseded by #189

@zth zth closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReScript v11
2 participants