NgComponent prevents react from rerendering when it should

[everdimension]

This is an issue to describe in a little bit more detail the problem described here: #88

The problem is the following:

If an object is mutated in angular, angular rerenders its views. But react2angular library uses ngcomponent lib under the hood, which prevents react from receiving updates (by making a shallow comparison of the old and new values).

As I understand, this line is the problem: https://github.com/coatue-oss/ngcomponent/blob/master/index.ts#L42

---

I created a demo to reproduce the bug: https://everdimension.github.io/reproduction-repo-react2angular/

Here's the code

[icfantv]

@everdimension I'm not sure if it's your sourcemaps or what (and I haven't pulled down your code to run locally), but I put a breakpoint in the else block of the isFirstRender check and it's never even getting called. In fact, the $onChanges isn't getting invoked again.

Additionally, I THINK it's only doing a shallow prop comparison, which is probably correct as a deep one could be arbitrarily expensive.

[icfantv]

Did some more digging here. I'm pretty sure your test is invalid. Read Todd Motto's blog post on AngularJS component lifecycle hooks, specifically $onChanges, but the crux of it is:

The golden rule to remember is, $onChanges is called in the local component controller
from changes that occurred in the parent controller. Changes that occur from the parent
which are inputted into a component using bindings: {} is the secret sauce here.

It's invoked initially from the first render (which is normal behavior) and then never again because the parent controller is not modifying anything in the bindings prop because you have neither.

[tony1377]

@icfantv I think @everdimension point is valid.

I'm not an angularjs expert but using the original test case above, if you add in a child angular component and provide it the same props as the react component you'll see that it updates but the react one doesn't.

Which seems to suggest to me that the problem lies in react2angular rather than the test case

[squidsoup]

Rather problematic for our app migration, as it mutates a collection when receiving websocket responses.

[everdimension]

@icfantv

I THINK it's only doing a shallow prop comparison, which is probably correct ...

It's not, though.

I think you either do not understand the react programming model or you haven't really tried to understand this particular problem. But the current behavior is incorrect.

Basically, current implementation of react2angular only creates something similar to a PureComponent, which is not sufficient for someone who migrated an angular app to react. While react discourages relying on mutations, it works well with them by rerendering whenever a parent rerenders, regardless of whether the props have changed or not. Using a PureComponent is opt-in and often not required (and sometimes can even be discouraged).

On the other hand, angular programming model relies heavily on mutations. Therefore, a safe interop cannot be possible if you only rerender react2angular components by shallow-comparing new props with the current props.

[icfantv]

@everdimension Or, perhaps my understanding of the issue was not what you'd intended? In the future, I would encourage you to be mindful of your tone and critique as my comments were not a personal attack on you, rather, it was my attempt at trying to understand the issue and either validate it (or not). Perhaps instead of saying your test (which makes it personal), I could have said the test (which does not). So that's on me.

All this aside, we've used this library for several years with nary an issue on several projects. This may mean nothing as maybe we've never used the library in the same as in your situation.

Finally, this was 4.5 months ago and I have no idea what I researched or how I did so so I can't speak anymore with any authority on whether or not what I was doing was correct (or not) or whether it accurately reflected your use case (or didn't).

[squidsoup]

As a point of comparison, ngreact supports a few different modes for change resolution:

watch-depth attribute indicates what watch strategy to use to detect changes on scope properties. The possible values for react-component are reference, collection and value (default)

https://github.com/ngReact/ngReact#the-react-component-directive

[texonidas]

@everdimension

On the other hand, angular programming model relies heavily on mutations.

I strongly disagree with this statement. Almost every single guide on components (which you have to be using for this library to be relevant) will tell you NOT to rely on mutation, and to explicitly disable two-way binding in favour of binding functions to the appropriate scope. You can't expect to convert a non-functional app (non-johnpapa angularjs) into a functional framework (react) without some legwork updating code.

[givehug]

Looks like this issue causes update blocking in react router https://reacttraining.com/react-router/web/guides/dealing-with-update-blocking

[tgfischer]

I've found a work around for now by using $timeout and angular.copy. For example, I'm doing something similar to the following

// In template
<my-component on-click="handleFooBar" />

// In directive
scope.handleFooBar = () => {
  scope.foo[i] = "bar";

  $timeout(() => {
    scope.foo = angular.copy(scope.foo);
  }, 0);
}

It's not an ideal solution though

[and-kost]

Hello everyone!
I faced the same issue. My temporary solution is using clone or cloneDeep function from lodash. I mean if we change the link of an object in the angularJS controller when the object is changed in the react component too.

[texonidas]

Revisiting this comment because I ran into this issue again from another angle. The crux of the solution is that whatever is being passed into the react components needs to be a new value.

For instance, where you may have had:
vm.someObj.someProperty = 5, with vm.someObj being passed as a prop, you would now need to do vm.someObj = { ...vm.someObj, someProperty: 5 }. The same applies for arrays, and those are the only two datatypes that I have run into this issue with.

The biggest I ran into making these conversions were places I relied on object equality comparisons rather than checking an ID property. Hopefully this helps others make the conversion over.

[arturgspb]

🤦‍♂️ I have a year to migrate a large application with hundreds of $watcher and a very large number of components with deep property changes. 😩

Maybe anyone in 2021 will need this workaround for no stop migrate to react:

angular
    .module('app')
    .directive('meHandlebars', function () {
        return {
            restrict: 'E',
            template: '<me-handlebars-react context="copyContext" source="source" blockStyle="blockStyle" inIframe="inIframe"></me-handlebars-react>', // proxy params
            scope: {
                source: '=?',
                context: '=?', // THIS VAR DEEP CHANGE IN MANY PLACES
                blockStyle: '=?',
                inIframe: '=?'
            },
            controller: function ($scope) {
                // CONTROLLER CODE MIGRATE TO REACT COMPONENT me-handlebars-react
                $scope.$watch('context', function (v) {
                    $scope.copyContext = _.cloneDeep(v); // lodash deep clone or other deep copy
                }, true);
            }
        };
    });

[kanapka94]

I had similar problem with refreshing React component when something changed outside in Angular. I just toggleed boolean variable with delay and used ng-if. React component was unmounted and mounted again. This behaviour forced React component to again reload state.

This is my pseudocode:

TopBar.html (Angular component template)

<Button ng-click="$ctrl.onClickSomething()">Click me</Button>

<div ng-if="!$ctrl.isTemporaryRefresh">
  <react-component></react-component>
</div>

TopBar.js (Angular component js)

onClickSomething() {
  this.refreshReactComponent();
}


refreshReactComponent() {
  this.isTemporaryRefresh = true;

  setTimeout(() => {
	this.isTemporaryRefresh = false;
  }, 10);
}