-
Notifications
You must be signed in to change notification settings - Fork 81
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
refactor: Rework earlier commit to annotate existing method #6452
Conversation
@@ -51,6 +52,7 @@ | |||
* exposed to api consumers, rather than wrapping in a Table type, as it handles the barrage stream and provides events | |||
* that client code can listen to. | |||
*/ | |||
@TsIgnore |
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.
Why do we need a TsIgnore or here? Anything in the get docs that mention best practices when a class inherits another class? I couldn't find any documentation there.
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.
This isn't really a GWT or JS thing, it is specific to the TS annotations and tooling we built (for DH, but can be used on any GWT project).
Java types are pretty rigid - the type is what it says it is, is has the members it says it has. By default, GWT then renames as appropriate for optimizing (methods with no override tend to be remade into static to make smaller, faster to call, and easier to inline, at the cost of the method name being bigger since it is globally scoped instead of instance scoped).
Jsinterop annotations then can force types/members to behave as entrypoints into those Java types, and prevent their signature from changing - this is sufficient to fix the bug here, by just decorating AbstractTableSubscription.close() with @JsMethod
. If the whole class was marked with @JsType
, all public methods would have been exported in this way, but that wasn't necessary/desired. No rhyme or reason is required for JS - just "does it have the method I think it does? okay great." If we had 1:1 java:js types with @JsType
on the abstract superclass then that would also give us an actual (prototype-based...) "class" in JS to go with it, but that requires that we have constructors that make sense in JS, which in this case would also mean providing exports for WorkerConnection, ClientTableSTate, and SubscriptionType. Moot point, since we also don't export dh.TableSubscription as a constructor function, nor TableViewportSubscription (and TreeSubscription is not exported at all to JS). All three "classes" are just "namespaces that hold events, and names that represent types which can be returned from other API calls", but what matters is that those instances have a close() method.
But we also want coherent typescript types in a .d.ts
file Now we need real names that mean something, whether or not we have accessible constructors or members. This design more or less requires "if a given Java type has exported members, then the type must be exported in some way" - but nothing useful comes from exporting AbstractTableSubscription. JS/TS consumers gain no useful knowledge (at this time) from those types having a common supertype, nor is it important that a third subclass exists. If we wanted AbstractTableSubscription to be visible, we'd have to give it a namespace and name (probably dh.AbstractTableSubscription), which either requires exporting the whole type via @JsType
, or just giving a hint to the TS wiring that "for the sake of generating good types, pretend that this is in the specified namespace". Alternatively, we can just mark the type as @TsIgnore
, which directs the tooling to move each member it finds down to any actually-exported subtype, for the sake of TS type defs and docs.
Note that this "move the member down" logic isn't specific to this annotation - it is (or was?) apparently required that if an interface defines a member in a .d.ts
file, any class that claims to implement that interface must also define that member, even though it obviously inherits it (and a .d.ts file contains no implementation, so specifying this isn't necessary).
Followup #6466