shirakaba / react-nativescript

React renderer for NativeScript
https://react-nativescript.netlify.com
MIT License
280 stars 14 forks source link

__ImplementsCustomNodeHierarchyManager__ can only be set to true when implementing CustomNodeHierarchyManager #42

Closed Lelelo1 closed 4 years ago

Lelelo1 commented 4 years ago

I am encountering a difficulty in implementing CustomNodeHierarchyManager interface in my rns-reactifiy trial. This is due to the reactify mixin class in my project will need to implement all interfaces and declare all methods currently in use by all rns components - and __ImplementsCustomNodeHierarchyManager__ can only be set to true.

I am not quiet sure of the consequence but I suspected that property enabled at all times is undesirable.

shirakaba commented 4 years ago

If you implement CustomNodeHierarchyManager, __ImplementsCustomNodeHierarchyManager__ needs to be true, because that's the fingerprint that the RNS Host Config will look for when examining an instance of a NativeScript Core UI element to determine whether it requires any custom handling.

It's used in this way in the Host Config:

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/client/HostConfig.ts#L414

And the implementsCustomNodeHierarchyManager() check is implemented here:

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/shared/HostConfigTypes.ts#L74

This is due to the reactify mixin class in my project will need to implement all interfaces and declare all methods currently in use by all rns components

Not all RNS components use it, mind. But either way – You can't optionally implement an interface, but what you can do is:

  1. Implement it (set __ImplementsCustomNodeHierarchyManager__ to true on any Reactified component)
  2. Also implement the methods of the CustomNodeHierarchyManager interface, but return false from each of them if you want to defer to the default Host Config implementation.

Note that this is an area that I intend to refactor dramatically, so it's best not to get too attached to this API!

Lelelo1 commented 4 years ago

Thanks for the info.

Note that this is an area that I intend to refactor dramatically, so it's best not to get too attached to this API!

No worries. It should be quiet easy to change or add new implementation in the way I am setting rns-reactify up.

Lelelo1 commented 4 years ago

I just recently realised I can access private and protected fields with Reflect. That way things that are protected and private can stay that way. Keep in mind interfaces can only declare public properties and...

Class 'Reactify' incorrectly implements interface 'MyInterface'. Property 'myProperty' is protected in type 'Reactify' but public in type 'MyInterface'


I assume it is an unwanted side effect that stuff in the CustomNodeHierarchyManager like __customHostConfigAppendChild? and __customHostConfigInsertBefore becomes public?

Screenshot 2019-10-03 at 15 24 17

So I suggested checking property with reflection instead:

const boolValue = Reflect.get(instance, "__ImplementsCustomNodeHierarchyManager__"); ... inside implementsCustomNodeHierarchyManager().

The class/reactify class can then declare the properties of CustomNodeHierarchyManager and stop implementing it - setting the properties to protected or private?

shirakaba commented 4 years ago

I assume it is an unwanted side effect that stuff in the CustomNodeHierarchyManager like __customHostConfigAppendChild and __customHostConfigInsertBefore becomes public?

No, actually I do want those to be public – the Host Config will be calling those methods, so they need to be public. It happens e.g. here:

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/client/HostConfig.ts#L416

I'm only writing the __ in front of the property names to be absolutely sure to avoid a name clash with any properties existing on the instance of the NativeScript Core UI element.

Lelelo1 commented 4 years ago

I will implement the interface then. The most optimaI I think would be to have them hidden from the developer on the rns component while having them accessible in host config. It might not be possible though - and it's only 4 properties after all.


All calls could be made with reflection though:

Screenshot 2019-10-04 at 09 34 53