emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

@tracked number not updating template in octane app #18038

Closed ASH-Anthony closed 4 years ago

ASH-Anthony commented 5 years ago

In the component below, totalWithDiscount and total are not updated in my template, despite the fact that if I add {{this.cart.total}} to the template, I can see the total change when removeItem is triggered.

export default class CartController extends Controller {
    @service cart

    @tracked tax = 0
    @tracked discount = 0

    get totalWithDiscount(){
        return (this.cart.total * ((100-this.discount)/100)).toFixed(2)
    }

    get total(){
        return (this.totalWithDiscount * (1 + (this.tax / 100))).toFixed(2)
    }

    @action
    removeItem(item){
       this.cart.remove(item)
    }
}

However, if I add a local cartTotal property and update that in the component, it works. This feels like an added step though.

export default class CartController extends Controller {
...
+ @tracked cartTotal =this.cart.total

    get totalWithDiscount(){
-        return (this.cart.total * ((100-this.discount)/100)).toFixed(2)
+       return (this.cartTotal * ((100-this.discount)/100)).toFixed(2)
    }

    get total(){
        return (this.totalWithDiscount * (1 + (this.tax / 100))).toFixed(2)
    }

    @action
    removeItem(item){
       this.cart.remove(item)
+     this.cartTotal = this.cart.total

    }
}

While I doubt the issue is in the service (remember, the template shows the updated total), here is the service

export default class CartService extends Service {
    @tracked items = [];
    @service store

    @mapBy('items', 'price') prices

    @sum('prices') total

    add(item) {
      this.items = [...this.items, item]
    }

    remove(item) {
        const newItems=this.items.filter(band => band.id !== item.id)
        this.items = newItems
    }

}

Am I missing something small? From reading the Octane docs, tracked and computed props in other classes should work just like tracked props in the component, right?

pzuraq commented 5 years ago

Hey @ASH-Anthony! I'm having trouble reproducing this locally, can you check to see which version of Ember Canary you are using? There have been lots of changes with tracked properties lately, so if you're using an older version of it it's possible that the issue has already been fixed. Thanks for reporting this!

ASH-Anthony commented 5 years ago

I'm using the latest canary release with an Octane blueprint and still seeing this issue. Here's my package.json

{
  "name": "octane-demo",
  "version": "0.0.0",
  "private": true,
  "description": "Small description for octane-demo goes here",
  "repository": "",
  "license": "MIT",
  "author": "",
  "directories": {
    "doc": "doc",
    "test": "tests"
  },
  "scripts": {
    "build": "ember build",
    "lint:hbs": "ember-template-lint .",
    "lint:js": "eslint .",
    "start": "ember serve",
    "test": "ember test"
  },
  "devDependencies": {
    "@ember/optional-features": "^0.7.0",
    "@glimmer/component": "^0.14.0-alpha.3",
    "babel-eslint": "^8.0.2",
    "broccoli-asset-rev": "^3.0.0",
    "ember-auto-import": "^1.2.20",
    "ember-cli": "github:ember-cli/ember-cli#1dc682ea479b084d7f03215c6c554138ed4bf76b",
    "ember-cli-app-version": "^3.2.0",
    "ember-cli-babel": "^7.7.0",
    "ember-cli-dependency-checker": "^3.0.0",
    "ember-cli-eslint": "^5.0.0",
    "ember-cli-htmlbars": "^3.0.0",
    "ember-cli-htmlbars-inline-precompile": "^2.0.0",
    "ember-cli-inject-live-reload": "^2.0.1",
    "ember-cli-sri": "^2.1.1",
    "ember-cli-template-lint": "^1.0.0-beta.1",
    "ember-cli-uglify": "^2.1.0",
    "ember-data": "github:emberjs/data#1df833396855d956b817540923dd89338463fec2",
    "ember-export-application-global": "^2.0.0",
    "ember-load-initializers": "^2.0.0",
    "ember-maybe-import-regenerator": "^0.1.6",
    "ember-qunit": "^4.1.2",
    "ember-resolver": "^5.1.3",
    "ember-source": "https://s3.amazonaws.com/builds.emberjs.com/canary/shas/1cb5f677e6d980b6e14986b5fdf1f98536f5a214.tgz",
    "ember-welcome-page": "^3.2.0",
    "eslint-plugin-ember": "^6.0.1",
    "eslint-plugin-node": "^8.0.1",
    "loader.js": "^4.7.0",
    "qunit-dom": "^0.8.0"
  },
  "engines": {
    "node": "6.* || 8.* || >= 10.*"
  }
}

P.S.: Thank you so much for your series of Octane posts. They've been a great resource for getting us ready for Octane.

ASH-Anthony commented 5 years ago

I found the fix, although I'm not sure if it's a bug or not, since, as I understand it, computeds and macros should be tracked. The issue was that my service used a macro for total; when I use a native getter, the template updates when the remove method is called. However, if I log the value of total in my service (when using macros), it gets updated correctly; the template itself does not update.

export default class CartService extends Service {
    @tracked items = [];

-    @mapBy('items', 'price') prices

-    @sum('prices') total
+    get total(){
+        return this.items.reduce((acc, item) => acc + item.price, 0)
+    }

    add(item) {
      this.items = [...this.items, item]
    }

    remove(item) {
        const newItems=this.items.filter(band => band.id !== item.id)
        this.items = newItems
    }

}
pzuraq commented 5 years ago

Yeah, the computeds definitely should be autotracked, and your original example should work. When I tried to recreate this locally, I set up everything the way you've described and copied the class code, but everything was updating correctly 😕 a reproduction would be very helpful, would definitely like to tracked this down.

amk221 commented 4 years ago

Can you point me in the direction of where I would write a failing test for this?

Because it's related to the use of tracked which, would be in the glimmer repo, but only when in combination with a macro, which would be the ember repo.

So I'm not sure...

pzuraq commented 4 years ago

This would be an Ember bug. Do you have a consistent reproduction @amk221 ?

amk221 commented 4 years ago

Yes, will post up a demo repo in a minute

On Wed, 12 Feb 2020 at 16:23, Chris Garrett notifications@github.com wrote:

This would be an Ember bug. Do you have a consistent reproduction @amk221 https://github.com/amk221 ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/emberjs/ember.js/issues/18038?email_source=notifications&email_token=AAC5I3ORYNDZF6L3XFXJOZLRCQPABA5CNFSM4HOF5CMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELRMVPA#issuecomment-585288380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC5I3MCD27EK4UPUQWDPCTRCQPABANCNFSM4HOF5CMA .

amk221 commented 4 years ago

https://github.com/amk221/-ember-tracked-macro/blob/master/app/components/my-component.js#L11

pzuraq commented 4 years ago

Computed properties and CP macros cannot depend on native getters directly unless you use @dependentKeyCompat.

See macro-decorators for a version that will work when depending on native getters (and all autotracking codepaths).

amk221 commented 4 years ago

So is this a different issue then?

pzuraq commented 4 years ago

It may be the same issue, but since we have not been able to reproduce this issue we can't know for sure unfortunately, I think we may be missing some context about @ASH-Anthony's app 😕

amk221 commented 4 years ago

Ok cool. Thanks for the link.

ASH-Anthony commented 4 years ago

Thanks for the follow-up guys. This was an old demo app using a pre-release of Octane. I can try it again with the latest version. Just working on some deadlines at work over the next 2 weeks and can get back at it after that and update this thread after that.

arthur5005 commented 4 years ago

@pzuraq hey, thanks for the info on @dependentKeyCompat macro,. I was pulling my hair out earlier today, glad I found this thread, that macro solved it.

It would be nice to see a mention of this incompatibility this part of the upgrade guide: https://guides.emberjs.com/release/upgrading/current-edition/tracked-properties/

It is unfortunate that this incompatibility exists though; it introduces a coupling, where the getters that are defined have to know about their use downstream.

I guess the best solution to avoid coupling is to migrate to macro-decorators, which actually looks quite lovely.

ASH-Anthony commented 4 years ago

This was an old demo project and when I updated it to use the latest stable version of Ember (using 3.16 and previous issue was a canary release), this works as expected, so I think this can be closed out. Thanks for your follow-up, everyone

pzuraq commented 4 years ago

Thanks @ASH-Anthony!