-
Notifications
You must be signed in to change notification settings - Fork 34
[interop] Add Support for Anonymous Declarations #434
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
base: main
Are you sure you want to change the base?
Conversation
@_i1.JS() | ||
external _AnonymousUnion_1189263 get responseObject; | ||
@_i1.JS() | ||
external _AnonymousConstructor_1059824 get productConstr; |
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 I may have miscommunicated earlier what I meant, but I was thinking of making these constructor values like
Product productConstr(
num id,
String name,
num price,
) =>
Product(
id: id.toDouble(),
name: name,
price: price.toDouble(),
);
instead. I'm not really sure it's useful for users to have this constructor as a getter (and it avoids the extra anonymous extension type if we don't), but I'm also totally okay with going the current route if you feel like it's better.
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.
- The constructor call shouldn't be a getter, it should be an override on
call
, so it is intended to be called asproductConstr()
. - I do not know the implications of having it as a variable vs as a function.
- I feel the anonymous extensions are useful, as in this example, they can be extended/implemented and typealiased.
Do you have any thoughts concerning these?
web_generator/test/integration/interop_gen/ts_typing_input.d.ts
Outdated
Show resolved
Hide resolved
}); | ||
|
||
test('Sub Type Interface Test', () { | ||
final a = InterfaceDeclaration( |
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'd consider a case where we check the LCA of three types where the pairwise LCAs are different than the LCA of all the types. This is to check we're not accidentally composing the LCAs (which we aren't).
e.g.
C -> B -> A
E -> D -> B
F -> H -> B
E -> G -> A
F -> G -> A
LCA{E, F} = G but LCA{C, E, F} (B) != LCA{C, LCA{E, F}} (A)
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, Nike! Just a couple more comments.
|
||
@override | ||
set isNullable(bool isNullable) {} | ||
bool isNullable; |
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 not make this late
and only set it when we have the value instead of defaulting like we do in line 32?
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.
We can, but it isn't guaranteed to be set. It is only set if isNullable
is set
/// isNullable can be null
Type _transformType(TSTypeNode type,
{bool parameter = false, bool typeArg = false, bool? isNullable}) {
// code
}
@@ -51,7 +46,7 @@ class BuiltinType extends Type { | |||
.where((p) => typeParams.length != 1 || p != $voidType) | |||
.map((p) => p.emit(options))) | |||
..url = fromDartJSInterop ? 'dart:js_interop' : null | |||
..isNullable = _isNullable ?? options?.nullable); | |||
..isNullable = isNullable || (options?.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.
In theory, isNullable
should have been set so we should not see any late initialization errors with the propose change above.
_ => BuiltinType.primitiveType(PrimitiveType.object, isNullable: false) | ||
}; | ||
} else if (cl case InterfaceDeclaration(extendedTypes: [final extendee])) { | ||
return switch (extendee) { |
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 guess the same question applies when cl
(which should maybe be called td
now :D) is a ClassDeclaration
as well.
final topoList = topologicalList(typeMaps.toList()); | ||
for (final level in topoList) { | ||
final typesAtLevel = commonTypes.intersection(level); | ||
if (typesAtLevel.isNotEmpty) { |
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'd just briefly leave a comment here mentioning that by definition, another least common ancestor can't be in the next level.
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.
Maybe I haven't hit a case, but I haven't encountered one so far. Is there an edge-case I'm missing?
return; | ||
} | ||
nodes.add(TypeHierarchalTree(list.first)..addValues(list.skip(1))); | ||
nodes.add(TypeHierarchy(list.first)..addChainedValues(list.skip(1))); | ||
} | ||
|
||
Set<String> expand() { |
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'd cache in this method as we use the result of this for intersections.
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.
Is there a good way of caching this and invalidating if nodes
is modified?
Fixes #385
Fixes #422
Fixes #410
Fixes #433
This PR adds support for generating declarations for anonymous declarations, such as anonymous objects, unions, closures and constructor types.
This PR also adds support for nullable types (types unioned with
undefined
and/ornull
).A hashing function is used for consistently hashing string objects, which is used for hashing identifiers for anonymous unions, objects and more, as well as comparing such names to allow reusing of such types.
string | number | boolean
may look like the following:JSTupleX
declaration generated depending on the number of members it may have (i.e if a 2 member tuple exists, it will be an instance ofJSTuple2<A, B>
). The tuple extension type looks like the following:call
. While Anonymous function types ((str: string) => void
) have theircall
implementations asexternal
, anonymous constructor types (new (str: string) => T
to constructT
) are implemented as function calls to create the given object. In the future after Make JSExportedDartFunction generic and add a toJSGeneric function sdk#54557, we should be able to replace this with a genericJSFunction
.