Hot reload seems unpredictable?

[spacesuitdiver]

New to Nativescript coming from React Native so maybe my expectations are a little different. I'm noticing a few things that make hot reloading really unpredictable to use.

1. When enabling a component prop like isScrollEnabled={false} and then removing it from the markup it doesn't reset back to true thus indirectly defaulting to scroll enabled.
2. Modifying the logic in something like an onTap callback of a button doesn't reflect code changes.

I'm just curious if I am doing something wrong or if this is just normal at the moment.

[shirakaba]

Heya! Thanks for using RNS 😊

Unlikely to be RNS’s fault for the simple boolean case, and I think even the onTap case should be fine; more likely because it’s a class component. react-hot-loader has problems with reloading various aspects of class components; the component may require a full unmount and remount before reflecting changes.

Another possibility is to alter the Webpack config to get hot reloading from the new React Fast Refresh or whatever it’s called (which React Native uses).

Gotta go for now; playing Age of Empires. Can look again later.

[shirakaba]

@spacesuitdiver Okay, I had a think about it (and incidentally I lost those two games of AoE2). Thank you very much for this minimal reproduction!

I believe the change to onTap will indeed be a react-hot-loader + class component interaction. Unfortunately, I think even Fast Refresh has issues with hot reloading certain parts of class components. Not sure whether field properties was one of them.

What's happening with the hot reload on isScrollEnabled is a different issue, and is partially RNS's fault (though entirely due to NativeScript not behaving like the web DOM, in that null isn't universally used as a way to reset properties to their default value).

• We diff the props during prepareUpdate in the React rendering lifecycle: https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/HostConfig.ts#L566
• This is the method we use to diff the props (a copy of what React DOM does, made relevant to NativeScript): https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/ReactNativeScriptComponent.ts#L279
• If a prop is removed from nextProps, e.g. isScrollEnabled then we push the key:value pair of ["isScrollEnabled", null] into updatePayload: https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/ReactNativeScriptComponent.ts#L373
• We then apply that updatePayload during commitUpdate: https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/HostConfig.ts#L601
• That calls into updateProperties, which calls updateDOMProperties here: https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/ReactNativeScriptComponent.ts#L500
• For that particular prop, we'd fall into the final conditional case in updateDOMProperties, which is setValueForProperty: https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/ReactNativeScriptComponent.ts#L274
• And so that'd simply execute this line here: https://github.com/shirakaba/react-nativescript/blob/d617cab1e373e4b217c28ebcf7e18896dd5a694f/react-nativescript/src/client/NativeScriptPropertyOperations.ts#L197

At that point, it's up to NativeScript Core (i.e. we'd be outside of the realm of React NativeScript) to decide what setting null on that particular property means. isScrollEnabled is defined on ScrollView here.

Now this is interesting. I hadn't thought about it before, but NativeScript uses this curious new Property() syntax to register a property. The construction object includes a defaultValue which is publicly accessible, but is sadly optional to define. In practice, if it's not defined, I believe it must default to undefined.

What could be done:

• If ever nextProps lacks a prop whose name existed in lastProps, we don't set to null (null makes sense for DOM, where setting something to null really does get you back to the default value). Instead, we set to __rns_default_value__.
• In the final case of setValueForProperty, we check whether the new incoming value is the magic string __rns_default_value__. If so, we'd check whether the Property defines a defaultValue. If so, we'd set that (rather than our usual null.
• If there is no defaultValue defined, then perhaps setting undefined would be more reasonable as a general case than setting null.

However, the tricky part is that I've encountered various Properties for which I've read in the implementation of its setter function that setting back to default should be done by setting the value to null. In those cases, I didn't look further to see whether the Property had a defaultValue configured (it probably didn't). Or maybe I'm just misremembering – certainly, in some of those cases, a strict null wasn't necessary, and any falsy value would have done.

So my only worry about changing this behaviour is that I think there's a large chance that in practice, many properties will have no defaultValue configured (due to laziness in the implementation), yet their setters might require null rather than undefined to set the value back to normal.

I think on the whole, doing it this new way would improve hot reloading by making more cases work, even if it makes some previously-working cases fail. I've experienced crashes when removing flexbox properties, because null isn't accepted as a default value. At least I can then point the blame at {N} Core for not properly implementing property unsetting, rather than having to blame the RNS renderer. It will certainly come down to case-by-case fixes, but this should catch more cases.

[shirakaba]

@spacesuitdiver Now implemented in the reset-props-to-default-value branch. Think I should test it on a few cases first before making the release, though!

[spacesuitdiver]

@shirakaba I'll have to figure out a clever way to install this from GitHub. I think because you ended up nesting the npm package into the react-nativescript folder it's not possible to install via shirakaba/react-nativescript#reset-props-to-default-value 😬

[shirakaba]

Ah yeah, it’s a pain. I should’ve chosen a different project structure. I chose the same structure as one of the other NativeScript flavours, so it made sense at the time.

For now, you’d have to clone the repo to somewhere on your filesystem, build RNS with npm prepare, and reference it in package.json with a “file:” descriptor, the same way that the sample project in the repo does. Sorry!

[shirakaba]

You might be able to install it via shirakaba/react-nativescript#reset-props-to-default-value after all by entering a Webpack alias. But it might also fail to build upon install, because the relevant package.json isn’t at the root of the repo 😣

[spacesuitdiver]

Trying to get the local built react-nativescript package is giving me some troubles with types. A slew of errors like this:

TS2717: Subsequent property declarations must have the same type.  Property 'view' must be of type 'SVGProps<SVGViewElement>', but here has type 'SVGProps<SVGViewElement>'.

What I have done is ran npm run prepare inside of the nested react-nativescript folder and then in my project I ran npm install <PATH_TO_NESTED_RN_FOLDER>.

[shirakaba]

@spacesuitdiver Very happy to say that this I've been able able to fix this, and hot reloading works like a dream (for the first time) thanks to your repro! Cases such as deletion of isScrollEnabled={false} will work beautifully now (I have a test case for it in PropDeletion.tsx)!

This fixes the long-standing errors I've faced with removing props on a FlexboxLayout or GridLayout, too, which is a really common use-case.

As for updating event handlers, I believe that any problems there must be due to react-hot-loader being incapable of updating fields on class components. I've confirmed that event handlers update fine upon hot reload in functional components:

export function UpdateEventHandlerTest() {
    function handleTap(args: GestureEventData) {
      console.log('1');
    }

    return (
        <$Button onTap={handleTap} text="click me"/>
    );
}

It's possible that migrating to Fast Refresh may help with that, though I'm not so sure. It's been mentioned as a fundamental limitation of React HMR in the past. I'd like to migrate to Fast Refresh at some point either way.

Anyway, for now:

npm install --save react-nativescript@0.23.0