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

Template does not get updated with tracked (computed) getter #18988

Open
iamareebjamal opened this issue May 20, 2020 · 13 comments
Open

Template does not get updated with tracked (computed) getter #18988

iamareebjamal opened this issue May 20, 2020 · 13 comments

Comments

@iamareebjamal
Copy link

I have a pretty weird bug which only happens on production build.

This is the component

import { tracked } from '@glimmer/tracking';
import classic from 'ember-classic-decorator';
import ModalBase from 'open-event-frontend/components/modals/modal-base';

@classic
export default class EventDeleteModal extends ModalBase {
  isSmall = true;
  @tracked confirmName = '';

  get isNameDifferent() {
    return this.eventName ? this.confirmName !== this.eventName : true;
  }
}

This is the template:

<div class="header">
  {{t 'Are you sure you would like to delete this event?'}}
  <div class="muted small text">
    {{t 'Deleting the event will delete all the data associated with it.'}}
  </div>
</div>
<div class="content">
  <form class="ui {{if this.isLoading 'loading'}} form" id="delete-form" autocomplete="off" {{action this.deleteEvent on='submit' preventDefault=true}}>
    <div class="field">
      <div class="label">
        {{t 'Please enter the full name of the event to continue'}}
      </div>
      <Input @type="text" @name="confirm_name" @value={{this.confirmName}} required={{true}} />
    </div>
  </form>
</div>
<div class="actions">
  <button type="button" class="ui black button" {{action 'close'}}>
    {{t 'Cancel'}}
  </button>
  <button type="submit" form="delete-form" class="ui red button" disabled={{this.isNameDifferent}}>
    {{t 'Delete Event'}}
  </button>
</div>

As you can see, the delete button should get enabled only when confirmName == eventName, which it does on debug build, in production build, it doesn't. I used ember devtools to check if tracked property isNameDifferent is being recomputed and it is, it changes to false when I type event name, but the button is not enabled. Now comes the weirdest part, when I added a console log statement in the getter:

get isNameDifferent() {
    console.log(this.eventName, this.confirmName, this.eventName ? this.confirmName !== this.eventName : true);
    return this.eventName ? this.confirmName !== this.eventName : true;
}

It starts working correctly, I tried multiple times and it only starts working in production build when I add this console.log statement. I thought this was a bug in autotracking but the computed property is recalculated anyway even when it doesn't update the template/HTML, so I have no idea why it is not working.

This started happening right after I migrated to tracked properties fossasia/open-event-frontend@2801505

Upstream Issue: fossasia/open-event-frontend#4383

Ember Versions:

"ember-cli": "~3.18.0",
"ember-source": "~3.18.1",
@iamareebjamal
Copy link
Author

Changing the getter to just

return this.confirmName !== this.eventName;

solves the issue but I still think it is a bug. Don't know why the tracked property was recomputed but UI didn't change though

iamareebjamal added a commit to iamareebjamal/open-event-frontend that referenced this issue May 20, 2020
For some reason, on previous conditions, property was recomputed,
but UI wasn't updated. that only happened in production builds as well.
Changing the condition slightly as a workaround worked.

Upstream Issue: emberjs/ember.js#18988
@rwjblue
Copy link
Member

rwjblue commented May 21, 2020

Was this.eventName being populated in both debug and prod builds (what is its value)?

@rwjblue
Copy link
Member

rwjblue commented May 21, 2020

I thought this was a bug in autotracking but the computed property is recalculated anyway even when it doesn't update the template/HTML

Just to confirm, what is "the computed property" referring to here? Do you mean the isNameDifferent getter? What was isNameDifferent before the tracked migration? A CP? If so, are there anything else that depends on it?

@iamareebjamal
Copy link
Author

  1. Yes, this.eventName is being populated for both. Its value is dynamically fetched from an API, this component is shown in a list
  2. Yup, isNameDifferent getter. The code was same, just had a computed decorator fossasia/open-event-frontend@2801505#diff-7d2ad7d6c2d307dea70c0d30cf4f922cR10. No, it doesn't depend on anything else as my comment mentions above that changing the getter to return this.confirmName !== this.eventName; fixes the problem, but it's not truly a fix but a workaround

@iamareebjamal
Copy link
Author

iamareebjamal commented May 21, 2020

I'd like to mention again that in both cases, the function is being re-evaluated on changing of this.confirmName as evident in devtools, and isNameDifferent is computed properly, just the UI is not updated

@pzuraq
Copy link
Contributor

pzuraq commented Jun 24, 2020

Is eventName tracked somehow here? I believe this is likely a case of short-circuiting.

In your example, you use a ternary statement which only returns the result if eventName is truthy. If the first time you access the property, eventName is falsy, then it will not run that portion of code, and never access this.confirmName. As such, confirmName will not be consumed, and changes to it will not cause updates.

This is logically sound because the only update that could cause changes here is if eventName changed. If eventName remained falsy, then the getter would always return false. So, tracking lets us skip over this code entirely in that case.

If eventName is not tracked, but does change, then I would say that is the root of the bug here. If eventName is an argument, you may need to explicitly mark it as @tracked on classic components for it to be consumed properly. Classic arguments are just assigned to the class using setProperties, so you need to access them with get() or mark them as @tracked for them to be consumable.

@iamareebjamal
Copy link
Author

iamareebjamal commented Jun 26, 2020

eventName doesn't change. It's static

Also, eventName is a prop passed from parent component

@lhz516
Copy link

lhz516 commented Oct 2, 2020

Hi @pzuraq ,

I don't see "short-circuiting" mentioned in the Ember docs. Is it a bug that will be fixed or is it something that developers need to avoid?

@pzuraq
Copy link
Contributor

pzuraq commented Oct 2, 2020

@lhz516 short circuiting is not mentioned directly, it's the outcome of applying the rules of autotracking. Autotracking is based on usage, so if you don't use something, then it will not be tracked. If you never end the else branch of an if statement, for instance. It is not a bug.

@lhz516
Copy link

lhz516 commented Oct 2, 2020

@pzuraq What if accessing some nested properties in the getter?

get val() {
  return this.a && this.a.b && this.a.b.c; // `c` is tracked
}

Is the only way to give both a and b a default value {} and access this.a.b.c directly?

get val() {
  return this.a.b.c;
}

@pzuraq
Copy link
Contributor

pzuraq commented Oct 2, 2020

@lhz516 if a is a property that can affect the output of get val(), then it should be a tracked property. That way, when it is originally accessed, even if it is undefined, it will be tracked. Later, when it is updated to be a new object, val will know to recompute.

@tniezurawski
Copy link
Contributor

@pzuraq Could you advise how one can debug short-circuiting? I have a very nested data structure to mimic a document (an email to be specific), and I'm trying to get rid of @computed, get, and set. With those, it works fantastic but we have to move forward and want to refactor it to getters and setters.

Structure is deeply nested:

image

So say one component changes the style's width (it only sees the style object) and another has to be triggered for re-rendering. I see the property changes properly when I debug from the Ember extension in the browser:

$E.templateSchema.rows[1].columns[0].blocks[0].descriptor.button.style.width
"59%"
// Change applied from the interface
$E.templateSchema.rows[1].columns[0].blocks[0].descriptor.button.style.width
"100%"

but the component responsible for applying styles doesn't see that change. Not in getters in JS-file nor in the template 🤷‍♂️

Thank you for any tip that has the potential to move me in right direction 🙏

@scottmessinger
Copy link

@tniezurawski I've run into something similar and I think the choices are:

  • Make the entire object a nested TrackedObject/TrackedArray so every array/object is tracked. Then you can use set a field like you've been doing (without set)
  • import {set} from "@ember/object" and use set(obj, field, val) when you update the property.

I don't know of a deepTracked that tracks everything automatically (similarly to what's used in ember-redux). Since that doesn't exist, we've been using POJOs for our data store and then use set to update the val. It's been working great.

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

6 participants