-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
support [@mel.unwrap]
in @@deriving jsProperties
#1162
base: main
Are you sure you want to change the base?
support [@mel.unwrap]
in @@deriving jsProperties
#1162
Conversation
> [@mel.unwrap] [@mel.optional] | ||
> other : [ \`String of int | \`Int of string ] option; [@mel.optional] | ||
> } | ||
> [@@deriving jsProperties, getSet] |
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.
Discussed with @anmonteiro offline. The reason why getSet
is needed is that regular / manual mutations of the field will be unsound / unsafe.
I suggested that we can check if mel.unwrap
is being used in a mutable
field and error out at parse time to prevent this. It would limit the functionality but probably avoid some confusion (or worse, runtime exns)
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.
I'm actually thinking: what does it mean to use a getter on a field that has @mel.unwrap
etc?
I think it'll just be unsound based on the object that has been created with a maker 🤔
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.
ad63bc5 shows that this feature is unsound even for <field>Get
, which makes me think that it's not something we can ship
One idea to fix this is to introduce a new extension point in melange that allows deriving |
fixes #1159