-
-
Notifications
You must be signed in to change notification settings - Fork 64
Add trivia for idlTypes #192
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
Conversation
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.
Looks good, just a question about nullable and null.
@@ -195,9 +195,13 @@ | |||
|
|||
const EMPTY_IDLTYPE = Object.freeze({ | |||
generic: null, | |||
nullable: false, | |||
nullable: null, |
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 you help me understand why we don’t default to false? Null seems a little odd here.
obj.nullable = !!consume("?"); | ||
const nullable = consume("?"); | ||
if (nullable) { | ||
obj.nullable = { trivia: nullable.trivia }; |
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 you help me understand why we don’t default to false?
@marcoscaceres I had to determine where should information about optional "modifier" tokens go, and the result is this.
I could do idlType.trivia.nullable === null
, but then we would have two fields about nullable token being absent, and causes inconsistency (some trivia fields sometimes being null, others never). Having two fields also may cause bugs as we should manage those simultaneously.
The null
way keeps compatibility with if (idlType.nullable)
and can also benefit from future optional chaining syntax: idlType.nullable?.trivia
.
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 clarification, makes sense.
These four commits are mutually related (so I couldn't fix writer.js for each commit), but still each commit has its own baseline changes.