Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Bindgen opt-in list #6639
Support Bindgen opt-in list #6639
Changes from all commits
c229976
dcf0618
354f07d
bbd81c2
e9683bf
cc73ada
7338f3f
a70af87
05a618f
8c7eeb0
3b888b4
420cd99
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 pretty sure it is impossible[1] to express duplicate fields in the spec file syntax right now, but I suppose it is fine to leave it incase that changes.
[1] You could have duplicates in the spec.yaml file, but the yaml spec requires that keys in a mapping are unique, so such a file isn't valid yaml. And even if the yaml parser doesn't detect that, since it parses into a Plain Old JS Object, any later field will completely replace the prior duplicate, so by the time you get to this code, you won't have any trace of the in-file duplication.
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 point!
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.
Starting a thread here to facilitate discussing this previous comment from @kraenhansen:
Regarding (A), we may want to handle the
isOptedInTo
differently. For instance, currently in our TS template (see RealmJS PR) we add deprecation annotations to the generated types with a message saying what to add to the opt-in list to be able to use it. This way we can still see the available methods and props when working with thebinding
, rather than switching to the spec to see if it's available. What's your take on this?Regarding (B), there were some discussions regarding the word "allow" and how it may be ambiguous in the sense that everything in the general spec is essentially allowed (and I guess "exposed" as well) to be used. "included" is also on the table and the current name "opt-in" is definitely easy to change 👍 I thought it made the intent a bit clearer than the other alternatives, but I'm very open to changing it to something that everyone finds suitable 🙂
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 agree that seems valuable 👍
My main gripe is that "opt-in" is two words and "opted in to" is three 🙈 But ... I'm not strongly opposed 🙂
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.
Thanks for the input, I'll leave the conversation unresolved for now to see if others want to weigh in on this 👍
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 have a slight preference for "includeList"/"isIncluded", but not enough to overrule the patch author. Just stating my preference in case it comes to a vote.