Immutable property values can be changed

[cjorasch]

Stencil version:

 @stencil/core@0.7.24

I'm submitting a:
[x] bug report

Current behavior:
When a component is rendered it is possible for immutable property values to change.

For example, the following component has properties and a state value that is calculated based on the properties.

@Component({
    tag: 'my-component'
})
export class MyComponent {
    @Prop() first: string;
    @Prop() last: string;

    private fullName: string;

    constructor() {
        log.info('constructor()');
    }

    componentWillLoad() {
        log.info('componentWillLoad()');
        this.fullName = this.first + ' ' + this.last;
    }

    componentWillUpdate() {
        log.info('componentWillUpdate()');
    }

    componentDidLoad() {
        log.info('componentDidLoad()');
    }

    render() {
        log.info(`render() first: ${this.first}, last: ${this.last}, fullName: ${this.fullName}`);
        return this.fullName;
    }
}

This component gets rendered from a parent container and later gets rendered again with different property values.

@Component({
    tag: 'my-container'
})
export class MyContainer {
    @State() first = 'John';
    @State() last = 'Smith';

    render() {
        return (
            <div>
                <ion-button onClick={() => (this.first = 'Mary')}>Change Name</ion-button>
                <my-component first={this.first} last={this.last} />
            </div>
        );
    }
}

During the first render the component behaves as expected:

my-component: constructor()
my-component: componentWillLoad()
my-component: render() first: John, last: Smith, fullName: John Smith
my-component: componentDidLoad()

After clicking on the button in the container component the child component is rendered again with changed property values:

my-component: componentWillUpdate()
my-component: render() first: Mary, last: Smith, fullName: John Smith

In this case the child component is assuming immutable property values and thinks it is ok to continue to use a calculated value that was based on the property values. In the example, the fullName state value is not valid.

The original instance of the child component is being re-used so the constructor and componentWillLoad methods are not called.

Expected behavior:

There are a couple of ways this could be handled:

• Create a new instance of the child component rather than re-using the component if the properties are immutable. This allows the child component to be self contained and assume immutable property values.
• Take away the assumption of immutable property values. Components need to be able to handle changes to property values at any time.

Currently it is possible to end up with very tricky errors when making the false assumption that an immutable property value will not change.

[manucorporat]

@cjorasch

When a component is rendered it is possible for immutable property values to change.

i am not sure where it's stated that Props are immutable, they are not at all. I believe that's part of your responsibility to handle it.

However, i like the idea of documenting good design patterns that help prevent this kind of bugs.

You can calculate fullname in the render() function or use the @watch() decorators to watch for changes in the props:

@Watch('first')
@Watch('last')
nameChanged() {
        this.fullName = this.first + ' ' + this.last;
}

or even better:

render() {
  const fullname = this.first + ' ' + this.last;
  return fullname;
}

[cjorasch]

The Stencil docs seem to indicate that props are immutable:

It's important to know, that @prop is by default immutable from inside the component logic. Once a value is set by a user, the component cannot update it internally.
However, it's possible to explicitly allow a @prop to be mutated from inside the component, by declaring it as mutable, as in the example below:
@Prop({ mutable: true }) name: string = 'Stencil';

If that is not really the case and they are always mutable then what is the purpose of the mutable setting? If you have to code for any property being changed then it is not clear why you would need to mark props as mutable or not.

The simple example of calculating fullName is trivial to handle as you suggested but complex components can be a lot harder to handle when there are lots of interrelated properties, private members and state values.

Examples from Ionic core

Taking a quick look at Ionic core it seems like there are many subtle cases where mutable props are not handled correctly. For example, in <ion-input>:

  componentWillLoad() {
    // By default, password inputs clear after focus when they have content
    if (this.clearOnEdit === undefined && this.type === 'password') {
      this.clearOnEdit = true;
    }
  }

In this case the clearOnEdit property will be set to a value if the type is "password" during the initial render of the component. If the type is initially "text" but gets changed to "password" then clearOnEdit is not set. This means that you get different behavior for the same set of property values. The reason this can cause problems is the change from "text" to "password" does not necessarily mean the app intentionally changed the property value - it may just be a by product of trying to render an entirely different input but rendering uses the existing component because it is in the same sequence in the DOM.

Problems can also exist in this component when using private property values. For example:

didBlurAfterEdit = false;

Here the value gets set when the component gets created and may later get changed to a different value. The problem occurs when the component is re-used with different property values. In this case didBlurAfterEdit may still be true when the parent component was trying to render an entirely different input element where this state should not be set.

I only looked at one component so there are probably many other subtle issues within ionic core.

Potential changes

The heart of the issue is that an existing component gets re-used if the parent renders the same component tag in the same DOM sequence. This makes it tricky to handle any state information (@Prop(), @State() or other component properties) correctly.

If the parent component is displaying a form with a bunch of child <ion-input> components and the form decides to hide one of the inputs then subsequent child components "shift up" so that the lastName input component becomes the password input component. Any existing state in the input component can then be incorrect.

It is, unfortunately, not clear how you would distinguish re-rendering a component (where state should be preserved) vs. moving a component in the DOM (where state should be moved) other than by being entirely stateless.

One possible improvement would be to enforce immutable property values as currently documented and create a new component instance if an immutable property value is changed vs. re-using an existing component if mutable property values are not changed. This would help with cases where "stale" state information might be re-used but would not handle preserving state when an element is moved.

In any case I think the mutable setting and somewhat enforcing no changes to property values from code does more harm than good because it decreases the chance of coding for changes to property values when any of those property values can be changed during rendering.

[felschr]

If that is not really the case and they are always mutable then what is the purpose of the mutable setting?

Yeah. I'd like to know that, too.
Perhaps they just added it in case they decide to add optional two-way binding later?