Skip to content

Fix tracked detection#1087

Merged
RobbieTheWagner merged 3 commits intoemberjs:masterfrom
patricklx:patch-3
Nov 22, 2019
Merged

Fix tracked detection#1087
RobbieTheWagner merged 3 commits intoemberjs:masterfrom
patricklx:patch-3

Conversation

@patricklx
Copy link
Copy Markdown
Collaborator

call getter directly
Ember.get auto tracks properties is seems, breaking the tracked detection.
So, it marks Getters as tracked, if the getter does not depend on any tracked property,

@patricklx patricklx force-pushed the patch-3 branch 2 times, most recently from 1015971 to 6757066 Compare November 18, 2019 14:19
@RobbieTheWagner
Copy link
Copy Markdown
Member

@pzuraq does this seem logical to you? I don't know about the internals.

@RobbieTheWagner
Copy link
Copy Markdown
Member

@patricklx if this fixes a bug in tracked detection, would it be possible to add a test covering that bug, that would fail before and passes now?

@pzuraq
Copy link
Copy Markdown
Contributor

pzuraq commented Nov 18, 2019

get should never mark a getter as anything. It will install a mandatory getter/setter on plain properties, but it does nothing unusual to plain getters. Can you describe the failure you were seeing in a bit more detail?

@patricklx
Copy link
Copy Markdown
Collaborator Author

its not that get marks it, but it adds the property tag into the tracking tags.
But we try to detect a tracked by checking if metal.track returns the same which metal.tagForProperty returns.
Which happens when accessing getters through get. https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/metal/lib/property_get.ts#L110

@pzuraq
Copy link
Copy Markdown
Contributor

pzuraq commented Nov 20, 2019

Ah, ok, makes sense. I think this is fine for now 👍

Copy link
Copy Markdown
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @pzuraq says it is fine, it seems fine to me 👍

@RobbieTheWagner RobbieTheWagner merged commit 02c9604 into emberjs:master Nov 22, 2019
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 1, 2020
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 5, 2020
patricklx added a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
@patricklx patricklx deleted the patch-3 branch October 22, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants