-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: allow adding additional members to production parse methods #745
Conversation
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
This is also due to microsoft/TypeScript#4628 which prevents changing the signature of static methods on inherited classes.
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.
Some nits:
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
@@ -16,8 +20,9 @@ export class CallbackInterface extends Container { | |||
tokeniser, | |||
new CallbackInterface({ source: tokeniser.source, tokens }), | |||
{ | |||
inheritable: !partial, | |||
inheritable: !!inheritable, |
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.
Can we make this change separately? We should either change all partial
to inheritable
everywhere or keep it everywhere, and making that change here sounds like out-of scope.
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.
Wasn't the issue here that callback interface doesn't support partial
at all?
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.
Oh hmm, I think we talked about it somewhere but can't find it anywhere, do you remember? I now wonder why we need it at all, neither the spec nor the mozilla-central uses inheritance for callback interfaces, right?
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.
Ah I found it, and I think we should just remove inheritable
s 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.
Thanks!
This PR adds the extMembers key to the production parse method's option bag. The extMembers key is used to add additional members that may exist during parsing. This is especially useful for allowing custom productions expand existing productions.
Additionally, this PR adds extensions to the parse method so that users can add additional allowedMembers on a per production basis.