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

Properties of Style interface do not accept string values #10

Closed shirakaba closed 4 years ago

shirakaba commented 4 years ago

As the ViewBase setter viewBaseInstance.backgroundColor = "gray"; is supported by proxying through to viewBaseInstance.style.backgroundColor, I think we also need to modify the Style interface to allow its properties to accept string type:

image

shirakaba commented 4 years ago

That same code screenshot, this time not covered up by the error popup:

image

halfnelson commented 4 years ago

I think that is starting to get out of scope for this project. We start leaving behind what you can do compared to nativescript core and it starts to become a "nativescript definition fixer". I'll have a think about it.

On Wed, 22 Apr 2020, 8:34 am Jamie Birch, notifications@github.com wrote:

That same code screenshot, this time not covered up by the error popup:

[image: image] https://user-images.githubusercontent.com/14055146/79920678-8dc0c300-8428-11ea-83ac-d8153ea023cc.png

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/halfnelson/nativescript-source-to-jsx-def/issues/10#issuecomment-617448069, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEGS5BGPK2EYDU66J7RMRLRNYNN7ANCNFSM4MNVIXWQ .

shirakaba commented 4 years ago

@halfnelson The main thing on my mind is that it is a valid method of writing styles:

image

It is also a hell of a lot more pleasant to write:

style={{
    backgroundColor: "gray",
    width: "100%"
}}

... than:

style={{
    backgroundColor: new Color("gray"),
    width: { value: 100, unit: "%" }
}}

If I try suggesting to React users that React NativeScript is a grown-up framework that's easy to use and has merits over React Native, I'll just get laughed out the door if they see how hard it is just to type in styles.

Svelte Native users would benefit from using the Style object with string properties, too. It provides intellisense and supports spreading a Style object in, so it has use-cases that the string-only style prop doesn't support.

halfnelson commented 4 years ago

For this case in particular, although it would be nice, nativescript has this code: https://github.com/NativeScript/NativeScript/blob/f2fb0976c1ea4a9ba5daef7ea7519c9e32f6d122/nativescript-core/ui/core/view-base/view-base.ts#L287 Which reads to me as only accepting string, otherwise it throws. The style class itself is also a class with some methods on it and not an interface so I would be hesitant to override their definition. It sounds like style handling in this manner might be framework specific

shirakaba commented 4 years ago

Which reads to me as only accepting string, otherwise it throws.

It sounds like style handling in this manner might be framework specific

Hmm. I see what you mean; I've been implementing special handling for it. But in fact, that's what React DOM does, too, so it's not unheard of.

I've been using it in this manner ever since the early days of RNS and have never run into a problem.

I feel it's strange that, if you want to segregate styles from the rest of your props, you have to resort to writing them with rich values, which takes much more typing and occasionally requires imports. It's a huge quality of life issue for users.

shirakaba commented 4 years ago

Okay, so there's some dissonance introduced here because RNS is doing some trickery under the hood.

It presents the style property with Style type, which is satisfied by the user passing in an object whose keys and values all conform to the Style interface. No expectation for them to instantiate a Style class.

RNS then iterates over that object and calls the setter on each field.

Object.keys(styles).forEach((styleName: string) => {
    viewBase.set(styleName, styles[styleName]);
});

So I'd concede that we are arguably getting into a framework-specific request here rather than fixing NativeScript Core's typings.

Could I introduce an option into the library to omit the style prop from ViewBase altogether so that I can augment it freely within RNS?