Skip to content

Use open poly variant to cover the FUTURE case #19

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
rickyvetter opened this issue Feb 25, 2020 · 6 comments
Closed

Use open poly variant to cover the FUTURE case #19

rickyvetter opened this issue Feb 25, 2020 · 6 comments

Comments

@rickyvetter
Copy link

rickyvetter commented Feb 25, 2020

Right now the SchemaAsset generated file uses a marker to signify that additional values could be added to the schema later so it is unsafe to exhaustively match. Could we instead use an open poly variant like so:

type t('a) = [> | `Case1 | `Case2 ] as 'a;`
@zth
Copy link
Owner

zth commented Mar 4, 2020

Sorry for missing this! You mean instead of including the FutureAddedValue case?

@rickyvetter
Copy link
Author

Yeah - exactly. I am hoping that with the poly variant + some PR like rescript-lang/rescript#3801 but for poly variant would allow for a 0 runtime version of SchemaAsset. This would mean you'd have the very large Reason file, but JS byte size would be unaffected.

@zth
Copy link
Owner

zth commented Mar 8, 2020

@rickyvetter I'm intrigued! And I agree that would be the best thing to do. However, I'd like to wait until that actually lands in BuckleScript before I change anything. Do you have any insight on ongoing work for that particular feature?

At that point SchemaAssets.re could also be removed (although I plan on putting a few other things in there, so it might not after all), which would be nice.

@zth
Copy link
Owner

zth commented Jul 23, 2020

@rickyvetter likely starting work on this soon given that BS is approaching shipping that feature from what I can see. SchemaAssets.re is gone too now, for other reasons as well.

@zth
Copy link
Owner

zth commented Jul 24, 2020

This is implemented here #90 although in a slightly different shape. Waiting a stable BS release with the feature, and it'll be merged after that.

@zth
Copy link
Owner

zth commented Aug 3, 2020

Fixed in e36948f 🎉

@zth zth closed this as completed Aug 3, 2020
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

No branches or pull requests

2 participants