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

(action (mut foo)) does not subscribe to foo, but (fn (mut foo)) does #18804

Closed
mwpastore opened this issue Mar 9, 2020 · 3 comments
Closed

Comments

@mwpastore
Copy link

mwpastore commented Mar 9, 2020

N.B. (mut) seems to be "moot"; the difference in behavior is present with or without it wrapped around the property in question.


This is real quirky, but seems to violate the principle of least astonishment, and could create some surprising edge cases and performance issues for unsuspecting developers.

While monitoring some property updates using the {{did-update}} render modifier to help troubleshoot another issue, I noticed a strange difference in behavior between (action (mut foo)) and (fn (mut foo)). This is obviously a contrived example—because you probably wouldn't use {{did-update}} this way—but it clearly illustrates the problem.

Consider the following test cases A and B:

A. (action (mut foo))

<Input @type="text" @value={{this.prop}} placeholder="Edit property value" />

<div {{did-update (action (mut this.propDidUpdate) true) this.prop}}>
  Property did update? <Input @type="checkbox" @checked={{this.propDidUpdate}} />
</div>

B. (fn (mut foo))

<Input @type="text" @value={{this.prop}} placeholder="Edit property value" />

<div {{did-update (fn (mut this.propDidUpdate) true) this.prop}}>
  Property did update? <Input @type="checkbox" @checked={{this.propDidUpdate}} />
</div>

Case A works as you'd expect: you enter some text in the input field and the checkbox becomes checked. You can then manually un-check the checkbox, and repeat ad nauseum.

Case B starts out fine: you enter some text in the input field and the checkbox becomes checked. But you can't un-check the checkbox! Trying to do so causes the {{did-update}} to re-fire and re-check it. (In Ember 3.14.3 via Twiddle this actually seems to blow up the stack.)

Like I said, quirky. Unfortunately, anybody diligently following the upgrade guides is going to rewrite their (action (mut foo)) to (fn (mut foo)) and likely have some new updates propagating throughout their app that weren't before. Maybe the reduced complexity of (fn) makes this impossible to resolve, and the ultimate solution is to stop using (mut) and start using ember-box or ember-set-helper?

@pzuraq
Copy link
Contributor

pzuraq commented Mar 9, 2020

I believe this is likely because whenever arguments that are passed to fn change, it will produce a new bound function, whereas action was lazy and would not do this. I'm not sure if we could change that behavior, since there may already be lots of users depending on it.

However, it is also definitely an anti-pattern to be mutating state directly via modifiers like this. Mutating state within Ember as a side-effect of a modifier or helper is very problematic, and will likely cause issues such as triggering the back-tracking assertion that was strengthened in 3.15, or invalidation issues that cause problems like this. This is effectively very similar to the same problems that observers had, and it's part of the reason they are no longer considered part of the programming model.

If you're relying on this pattern to get things done, I'd love to dig in a bit more so we can understand your use-case better and try to come up with an alternative pattern.

@mwpastore
Copy link
Author

mwpastore commented Mar 9, 2020

@pzuraq Always a pleasure to have you respond to my issue reports!

However, it is also definitely an anti-pattern to be mutating state directly via modifiers like this.

Agreed. It's a contrived example, designed to clearly show the behavior difference(s) between the two approaches. (And it just so happened to be the "technique" I was using to troubleshoot an unrelated issue, wherein I discovered this "problem.") I could have easily used {{log}} or an @action instead to demonstrate the issue.

I don't mute state via render modifiers in my actual apps, nor would I suggest it to others. 😄

I believe this is likely because whenever arguments that are passed to fn change, it will produce a new bound function, whereas action was lazy and would not do this. I'm not sure if we could change that behavior, since there may already be lots of users depending on it.

That tracks. I figured as much because I know the complexity of (fn) is greatly reduced cf. (action), which makes it desirable. But I may not be so quick to use it for inline methods going forward, knowing now that it eagerly binds to its arguments and might propagate unwanted updates. Maybe this is a documentation problem?

@sandstrom
Copy link
Contributor

I'm doing some issue gardening 🌱🌿 🌷 and came upon this issue. It seems outdated and likely no longer relevant.

By closing some old issues we reduce the list of open issues to a more manageable set. Let me know if you think this is a mistake and that the issue should stay open.

@sandstrom sandstrom closed this as not planned Won't fix, can't repro, duplicate, stale Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants