Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: mutating an immutable property calls related watcher functions w/ new value #3490

Open
3 tasks done
kelvindart opened this issue Jul 27, 2022 · 1 comment
Open
3 tasks done
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.

Comments

@kelvindart
Copy link

Prerequisites

Stencil Version

2.17.2

Current Behavior

When decorating a function with a @Watch decorator for an immutable property, and changing that property internally, the watcher function is still called despite the property being immutable.

Example

I have the following immutable (by default) property and watcher on that property:

  @Prop() myVar: string = 'foo';

  @Watch('myVar')
  watchMyVar(newValue: string, oldValue: string): void {
    console.log('---watchMyVar START---');
    console.log(newValue, oldValue);
    console.log('---watchMyVar END---');
  }

If I have a method which mutates myVar internally, the watcher function is subsequently called.

mutateImmutableProperty(newVal: string): void {
  this.myVar = newVal;
}

If I then call mutateImmutableProperty, watchMyVar is subsequently called because the value of myVar is updated:

this.mutateImmutableProperty(`${Date.now()}`);

// ---watchMyVar START---
// bar foo
// ---watchMyVar END---

However, myVar is immutable (mutable: false - by default) therefore, I would question if it's correct to call subsequent watcher functions.

Expected Behavior

For the @Watcher functions to not be called on immutable properties when they are modified internally.

Steps to Reproduce

  1. Create a Stencil component which has a property defined as follows:
@Prop() myVar: string = 'foo';
  1. Create a watcher on that property:
@Watch('myVar')
watchMyVar(newValue: string, oldValue: string): void {
  console.log('---watchMyVar START---');
  console.log(newValue, oldValue);
  console.log('---watchMyVar END---');
}
  1. Create a method to mutate the object:
mutateImmutableProperty(newVal: string): void {
  this.myVar = newVal;
}
  1. Call this method internally. I have attached this to the onClick event of a button:
render() {
  return [<div>{this.myVar}</div>, <button onClick={() => this.mutateImmutable(`${Date.now()}`)}>Mutate</button>];
}
  1. Observe the console logs with the following output, illustrating the watcher function being called:
---watchMyVarHandler START---
1658921136478 1658920723269
---watchMyVarHandler END---

Code Reproduction URL

https://github.com/kelvindart/stencil-watcher-bug

Additional Information

Screenshot 2022-07-27 at 12 18 33

@ionitron-bot ionitron-bot bot added the triage label Jul 27, 2022
@alicewriteswrongs alicewriteswrongs changed the title bug: bug: mutating an immutable property calls related watcher functions w/ new value Jul 27, 2022
@alicewriteswrongs
Copy link
Contributor

@kelvindart thank you for filing this detailed issue, and for providing a nicely put together minimal reproduction! it's really really appreciated :)

I have tried out your reproduction case and confirmed that this is an issue. This behavior where 'immutable' @Props aren't exactly all that immutable is something I've observed before as well.

If you're interested in taking a look, the behavior to print the warning message was added in #2779 in order to fix #2433, but although a dev-mode warning has been added we haven't ever made a change to Stencil to actually prevent mutating these properties.

At present we need to do some refinement to figure out exactly what the behavior here should be, in light of the fact that the current behavior is that @Props without mutable: true are, in fact, fairly freely mutable. We need to make sure that any change we make here doesn't break how Stencil is currently being used. Accordingly, I'm going to mark it for further refinement and ingestion into our backlog.

Thanks again for filing this issue and bringing this to our attention! The detailed writeup and reproduction are much appreciated 😄💯👍

@alicewriteswrongs alicewriteswrongs added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Jul 27, 2022
@ionitron-bot ionitron-bot bot removed the triage label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

No branches or pull requests

2 participants