-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix any nullability and refactor typing model #88
Conversation
Quite a large diff in the output but it's entirely either the |
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 great, only very minor optional suggestions below
if (type1 == 'JSUndefined' || type2 == 'JSUndefined') { | ||
// This sentinel is only for nullability. | ||
return _RawType(type1 == 'JSUndefined' ? type2 : type1, true); | ||
} else if (type1 == type2) { |
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.
style nit: consider removing the else
here. Mostly it helps make more visible the conditions that are excluded from the rest of the method.
if (type1 == 'JSUndefined' || type2 == 'JSUndefined') {
return ...;
}
if (type1 == type2) {
return ...
}
If by chance we can make them fit as a single line, it would make it visibly more obvious too.
It may be possible if we split the undefined cases and add a few more locals:
final type1 = rawType1.type;
final type2 = rawType2.type;
final nullable1 = rawType1.type;
final nullable2 = rawType2.type;
if (type1 == 'JSUndefined') return _RawType(type2, true);
if (type2 == 'JSUndefined') return _RawType(type1, true);
if (type1 == type2) return _RawType(type1, nullable1 || nullable2);
// ...
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 is more readable. I still don't love how the logic places the equality case in-between the two sentinel cases, but alas.
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.
mmm... good point. If we can guarantee that every time we create a _Ratype
with 'JSUndefined'
we also use true
for the nullability, then we could move the ==
first. WDYT?
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 like it, prevents a non-nullable JSUndefined
from leaking into a _RawType
. #89
var nullable = type.nullable; | ||
|
||
// Convert JS types to primitives. | ||
if (dartType == 'JSBoolean') { |
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.
style nit: this may be a nice use of the new pattern syntax :)
dartType = switch(dartType) {
'JSBoolean' => 'bool',
'JSString' => 'String',
'JSInteger' => 'int',
'JSDouble' => 'num',
'JSNumber' => 'num',
'JSUndefined' => 'void',
_ => dartType,
};
if (dartType == 'void') nullable = false;
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.
Done, thanks!
Major changes: 1. Fixes an issue where we assume that all JSAnys should be nullable, whereas all anys should be nullable instead. 2. Changes typing model to go from IDLType to "raw" type, which is either an IDL declaration type or a dart:js_interop type, to a Dart type reference. This makes it easier to add generic types later as well. Minor changes: 1. Reuses _Type and renames it to _RawType instead of using a record. 2. Removes unnecessary "isReturn" parameters as undefined can only appear in a return type per the web IDL spec. 3. JSVoid -> void as it's more readable.
Major changes:
Minor changes: