Add forceUpdate method to Component

[robframpton]

No description provided.

[jbalsas]

Possible typo a an update?. Also, maybe unnecessary space in the second line?

[jbalsas]

As per React, forceUpdate takes a callback... probably because it works asynchronously... should we take one as well? Do we have usage examples of this?

[robframpton]

It won't hurt to add the functionality, though technically someone could achieve this by listening to the next rendered event.

component.once('rendered', function() {
  console.log('just force updated');
});
component.forceUpdate();

Though I think we should add the callback even if it's just for cleanliness.

component.forceUpdate(function() {
  console.log('just force updated');
});

[jbalsas]

Can we explain what to be force updating means a bit more? Not sure if it is common knowledge, but maybe something along the lines, wether it should ignore shouldUpdate and always render... , something that doesn't include the variable name :)

[jbalsas]

Similar to the other comments about this :)

[robframpton]

@jbalsas, updated.

[eduardolundgren]

What's the goal of this forceUpdate?

[jbalsas]

From React.Component#forceUpdate:

By default, when your component’s state or props change, your component will re-render. If your render() method depends on some other data, you can tell React that the component needs re-rendering by calling forceUpdate().

Calling forceUpdate() will cause render() to be called on the component, skipping shouldComponentUpdate().

This can pretty much always be bypassed by a proper state/prop modification, but here's a simple dummy example using Math.random() (not claiming this is super useful 😂 )

class App extends React.Component{
  constructor(){
    super();
    this.forceUpdateHandler = this.forceUpdateHandler.bind(this);
  };
  
  forceUpdateHandler(){
    this.forceUpdate();
  };
  
  render(){
    return(
      <div>
        <button onClick= {this.forceUpdateHandler} >FORCE UPDATE</button>
        <h4>Random Number : { Math.random() }</h4>
      </div>
    );
  }
}

[eduardolundgren]

I see, that's cool.

[jbalsas]

Hey @Robert-Frampton, this looks good in general, I have only one concern about how the forceUpdating flag is being managed which might be too convoluted since I haven't even been able to put it into a failing test 😂

it('should clean the isForceUpdating if render fails for some reason', function() {
	var CustomComponent = createCustomComponentClass((component) => {
		// Something can happen that prevents a successful render?
		// throw new Error('cant render');
	});

	comp = new CustomComponent();

	var listener = sinon.stub();

	comp.forceUpdate(listener);

	assert.equal(listener.callCount, 0);
	assert.isFalse(comp.isForceUpdating());
});

it('should not call the forceUpdateCallback if render fails for some reason', function() {
	var failRender = true;
	var CustomComponent = createCustomComponentClass((component) => {
		if (failRender) {
			// Something can happen that prevents a successful render?
			// throw new Error('cant render');
			failRender = false;
		} else {
			component.informRendered();
		}
	});

	comp = new CustomComponent();

	var listener = sinon.stub();

	comp.forceUpdate(listener);

	assert.equal(listener.callCount, 0);
	assert.isFalse(comp.isForceUpdating());

	comp.render();

	assert.equal(listener.callCount, 0);
	assert.isFalse(comp.isForceUpdating());	
});

My concern is that something may happen during a render phase that prevents us from cleaning up, and then we'll carry that information to a subsequent render...

Do you think that's possible and/or preventable?

[robframpton]

Hey @jbalsas

This is actually slightly related to what I was hoping to implement for #281.

Right now it's pretty hard to catch errors that originate from incremental dom. For example, if you put something like this in your template...

{template .render}
	<div></span>
{/template}

And try to catch the error by doing...

forceUpdate(opt_callback) {
	this.forceUpdateCallback_ = opt_callback;
	this.forceUpdating_ = true;

	try {
		this.updateRenderer_();
	} catch (e) {
		this.forceUpdateCallback_ = null;
		this.forceUpdating_ = false;
	}
}

It won't actually catch it, the error just bleeds through due to how we are invoking incremental dom.

I think we can accomplish better error handling, but I think it's outside the scope of this branch. I'm fine with holding off on these changes until we improve error handling if you'd like.

[jbalsas]

Hey @Robert-Frampton, that's fine, I was thinking more along the lines of

forceUpdate(opt_callback) {
    this.updateRenderer_({
        forceUpdate: true,
        updateCallback: opt_callback
    });
}

If there's a way to push that down the line, we won't hold state in the components and avoid these type of issues. It should also be easiest to test since there are no side-effects or additional coupling between renderer and component.

Not sure if it's doable, though 🤷‍♂️

[robframpton]

Oh I see, yeah I can look into it.

[robframpton]

Hey @jbalsas

I was able to remove the forceUpdating_ property to help simplify, but I can't remove the forceUpdateCallback_ without major changes. However, I added this check, which should ensure that if rendering fails during a force update for some reason, the next time the component updates it will delete any left over callback.

On a somewhat unrelated note, I noticed that the update method wasn't even consuming the data being passed to it, but I would like to address this in a separate issue.

[jbalsas]

Merged!