halfnelson / nativescript-source-to-jsx-def

Walk nativescript source to generate JSX types for `svelte-type-checker-vscode`
Other
4 stars 1 forks source link

Various missing on<Property>Change event handlers #16

Open shirakaba opened 4 years ago

shirakaba commented 4 years ago

For one example:

WebView has the src attribute, and yet lacks representation of an onSrcChange event handler in the typings.

All Properties in NativeScript emit a "change" event, named "onMyPropertyChange" (for the Property "myProperty") upon changing. I think the code for this is in the Property or Observable class.

While with reactive frameworks we aren't generally too interested in listening for property change events (because it's usually us causing the change in the property), src is a good example of a strictly necessary change event to listen to, as the WebView itself will (I believe) update its src when navigating to new pages.

The odd thing is that evidently a large number of Property change event handlers are supported in the typings (e.g. DockLayout's onStretchLastChildChange). Wonder what's going on there.

halfnelson commented 4 years ago

There is code that looks for all the properties and generates the onchange event handlers My guess is that the way Webview does it, it isn't via a Property that is registered with Register

halfnelson commented 4 years ago

nope, it seems it does register, maybe it is being shadowed by something

shirakaba commented 4 years ago

Some differences that come to mind:

  1. in the case of WebView's src Property, it's being registered upon WebViewBase, which is an abstract class, unlike how the defaultPage property is registered for Frame (upon a concrete class).
  2. in the case of WebView's src Property its registration is not performed on the immediate line after the call to new Property(), again unlike defaultPage.

https://github.com/NativeScript/NativeScript/blob/446163d3f890af19fbcda341b3aa7f1eb8efa082/nativescript-core/ui/web-view/web-view-common.ts#L102

https://github.com/NativeScript/NativeScript/blob/446163d3f890af19fbcda341b3aa7f1eb8efa082/nativescript-core/ui/frame/frame-common.ts#L716-L721

shirakaba commented 4 years ago

@halfnelson Some more debugging.

The file:

/Users/jamie/Documents/git/nativescript-source-to-jsx-def/nativescript_src/nativescript-core/ui/web-view/web-view.d.ts

Is only visited by NativeScriptCoreJSXExporter.ts during the getPropertiesFromSkippedParent() function, and is never visited during getPropertyRegistrations().

I found this with the conditional breakpoint baseClass.getName().indexOf("WebView") !== -1:

image

I'm not familiar with everything end-to-end so I'm not sure how to approach fixing it, but maybe any Properties found from a "skipped parent" (such as src from WebViewBase) don't get the onSrcChange type generated? Does that sound plausible?

halfnelson commented 4 years ago

Unfortunately I think the src it finds on your break point is the "src: string" not the dynamic property created by Register (which is why it doesn't get an event) Upon investigation of the web-view class, I noticed that my IDE couldn't determine the type of the srcProperty at registration time in web-view-common.ts. After I change the property construction code to export const srcProperty:Property<WebViewBase, string> = new Property<WebViewBase, string>({ name: "src" }); The jsx exporter detects it again as a dynamic property. I think the problem is the "find usages of register" call I was making, couldn't see it (since it was inferred as any)

The work around I though of was to fall back to the the alternate propertyRegistration detection logic I have for plugins, which looks for the declaration in the .d.ts files like: public static drawerContentSizeProperty: Property<RadSideDrawer, number>; unfortunately we are foiled again here, since the property does not seem to be declared correctly we see it as export const urlProperty: Property<WebView, string>; which I think is just a missed rename.

Are there any other instances where the property isn't being picked up? otherwise it might just be easier to hardcode this one too

shirakaba commented 4 years ago

@halfnelson Thanks for digging into this. As you show, lack of the suffix "Property" on the instantiated Property may indeed be throwing it off.

While we could certainly hard-code it, I think that this affects a good number of registered Propertys.

FlexboxLayout is one. Each of its attributes are a Property, and yet none have the on*Change event handler typed:

https://github.com/NativeScript/NativeScript/blob/446163d3f890af19fbcda341b3aa7f1eb8efa082/nativescript-core/ui/layouts/flexbox-layout/flexbox-layout-common.ts#L229-L230

This is a complex case wherein well-named Property instances such as alignItemsProperty exist alongside getters and setters (e.g. for alignItems).

halfnelson commented 4 years ago

Those don't look quite the same. Those dynamic props look like they are registered against style not flexboxlayout

On Mon, 18 May 2020, 7:45 pm Jamie Birch, notifications@github.com wrote:

@halfnelson https://github.com/halfnelson Thanks for digging into this. As you show, lack of the suffix "Property" on the instantiated Property may indeed be throwing it off.

While we could certainly hard-code it, I think that this affects a good number of registered Propertys.

FlexboxLayout is one. Each of its attributes are a Property https://github.com/halfnelson/nativescript-source-to-jsx-def/blob/cff268f010c11fd6001b43ea63ef032c187e9ad9/react-nativescript-defs/react-nativescript-jsx.ts#L196-L203, and yet none have the on*Change event handler typed.

https://github.com/NativeScript/NativeScript/blob/446163d3f890af19fbcda341b3aa7f1eb8efa082/nativescript-core/ui/layouts/flexbox-layout/flexbox-layout-common.ts#L229-L230

This is a complex case wherein well-named Property instances such as alignItemsProperty exist alongside getters and setters (e.g. for alignItems).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/halfnelson/nativescript-source-to-jsx-def/issues/16#issuecomment-630071184, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEGS5EZWCD6ZMZVCVPTCTLRSD7UNANCNFSM4MPTZNWQ .

shirakaba commented 4 years ago

Ah, whoops. You're right.

I'm trying to track down other possible instances of on*Change missing but I'm finding it tough to find any. It does make me reflect on how weird NativeScript is in that a good half of the properties are bona fide registered Propertys while the other half are just setters on the class. So half the framework's properties support two-way binding (I think that's what it's all for) and half just don't.

I feel like @vakrilov would know why some are one way and others are the other. There must have been some criteria.