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

[BUGFIX][FEAT][Tracked Properties] Adds Arg Proxy Feature Flag #17835

Merged
merged 2 commits into from
Apr 22, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 2, 2019

Currently, we pass a captured snapshot of arguments to the custom
component manager and expect users to update their args and trigger
notifications, dirtying any getters that rely on args. This is
problematic for a few reasons:

  1. It's fundamentally less efficient, since every getter on a component
    will invalidate any time any of the arguments on the component
    changes. This may not be a huge deal in most components, but it could
    be problematic at scale.
  2. Upstream components will currently rerender if anything changes
    downstream of them.

The second problem is more of an issue here. The crux of the issue is
that as we are rendering, we must somehow dirty the args object in
order for getters to invalidate. Currently, there are two ways of doing
this:

  1. Dirtying the tag and bumping the global revision count. This results
    in the component itself being dirtied, which causes arguments to be
    assigned again on the next render, which dirties everything again.
  2. Setting the argument's tag to CURRENT_TAG, as we do in Glimmer.js.
    This always returns the latest revision though, which means that we
    are always dirty.

This issue can crop up in many forms and leads to infinite rerender bugs
when it does.

The solution proposed here is to instead use a Proxy (or an object with
manually defined getters on platforms which do not support Proxy) to
wrap args. This allows us to directly access the references that
represent the arguments, and push their tags onto the autotrack stack as
accessed. Everything entangles properly, and we only rerender or
recalculate a getter when needed.

Alternatively, we could add a way to set the args tag to exactly
the current revision as the component is rendering. This would solve the
problems of upstream invalidations, but would not solve the issue of all
getters on a component invalidating each render.

Fixes #17799

@pzuraq pzuraq requested review from tomdale, rwjblue and krisselden April 2, 2019 03:12
@MelSumner MelSumner mentioned this pull request Apr 2, 2019
@pzuraq pzuraq force-pushed the bugfix/infinite-rerender/unstable-args branch 3 times, most recently from 6de8129 to e4d7460 Compare April 2, 2019 19:39
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Seems good to me, and given that its behind a feature flag it should be easy to roll back if we end up wanting to.

👍

@pzuraq pzuraq force-pushed the bugfix/infinite-rerender/unstable-args branch 2 times, most recently from 3a8c092 to d5579ca Compare April 4, 2019 16:02
@cibernox
Copy link
Contributor

cibernox commented Apr 7, 2019

I believe this may fix a problem I've experienced porting ember-basic-drodown to glimmer. I have a CP that depends on args.{top,left,width,others} that is not recomputing when those values change:

Copying explanation from @pzuraq:

- ahhh
- I know why it is
- we don't actually use set('args'), we use a setter
- so we don't actually notify args, we notify __args__
- autotracking will work correctly though, it's odd your CP isn't autotracking
- ah, but you probably haven't turned on autotracking
- k
- yeah, so the fixes/updates we have to do, we'll definitely address this

In case this help creating a test that validates the fix

@cibernox
Copy link
Contributor

What's the state of this? I can help verify that it fixes my issue upgrading my addons

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 11, 2019

@cibernox there will likely need to be a little additional work for it to fix your issue, we're working on a followup to the Tracked Properties RFC that will address some core issues we've found with interop between computed properties and tracked properties at the moment. This is semi-related, but on its own it likely won't fix your issue just yet.

@rwjblue rwjblue added the Octane label Apr 11, 2019
@cibernox
Copy link
Contributor

I can try to add a failing test for my issue if that helps, I think I've isolated it. Idk if it should be in ember or in glimmer

@pzuraq pzuraq force-pushed the bugfix/infinite-rerender/unstable-args branch from d5579ca to 71ac2ec Compare April 19, 2019 22:31
if (DEBUG) {
handler.set = function(target, prop) {
assert(
`You attempted to set ${target}#${String(
Copy link
Member

Choose a reason for hiding this comment

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

We chatted a bit about this, but I'd like to get the component itself into this message target is going to be the empty {} POJO you make just above. Can we update this to definition.ComponentClass.class instead? At least it would toString the component name...

@pzuraq pzuraq force-pushed the bugfix/infinite-rerender/unstable-args branch from 71ac2ec to 88ac5c1 Compare April 22, 2019 18:42
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

LGTM, lets land once CI is green...

Chris Garrett added 2 commits April 22, 2019 12:49
Currently, we pass a captured snapshot of arguments to the custom
component manager and expect users to update their `args` and trigger
notifications, dirtying any getters that rely on `args`. This is
problematic for a few reasons:

1. It's fundamentally less efficient, since every getter on a component
   will invalidate any time _any_ of the arguments on the component
   changes. This may not be a huge deal in most components, but it could
   be problematic at scale.
2. Upstream components will currently rerender if anything changes
   downstream of them.

The second problem is more of an issue here. The crux of the issue is
that as we are rendering, we must somehow dirty the `args` object in
order for getters to invalidate. Currently, there are two ways of doing
this:

1. Dirtying the tag and bumping the global revision count. This results
   in the component itself being dirtied, which causes arguments to be
   assigned again on the next render, which dirties everything again.
2. Setting the argument's tag to `CURRENT_TAG`, as we do in Glimmer.js.
   This always returns the _latest_ revision though, which means that we
   are _always_ dirty.

This issue can crop up in many forms and leads to infinite rerender bugs
when it does.

The solution proposed here is to instead use a Proxy (or an object with
manually defined getters on platforms which do not support Proxy) to
wrap `args`. This allows us to directly access the references that
represent the arguments, and push their tags onto the autotrack stack as
accessed. Everything entangles properly, and we only rerender or
recalculate a getter when needed.

Alternatively, we could add a way to set the `args` tag to _exactly_
the current revision as the component is rendering. This would solve the
problems of upstream invalidations, but would not solve the issue of all
getters on a component invalidating each render.
@pzuraq pzuraq force-pushed the bugfix/infinite-rerender/unstable-args branch from 88ac5c1 to af9ddaf Compare April 22, 2019 19:50
@rwjblue rwjblue dismissed krisselden’s stale review April 22, 2019 21:02

Concerns addressed...

@rwjblue rwjblue merged commit 06c0cd0 into master Apr 22, 2019
@rwjblue rwjblue deleted the bugfix/infinite-rerender/unstable-args branch April 22, 2019 21:03
@basicBrogrammer
Copy link

I'm running in to this issue .... at first i thought i had to do @computed('args.nameOfArg') but that didn't cause a rerender. When i remove the computed decorator it gives me the Max Stack. Is this the same issue? If so, how do i update my ember octane preview module?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop/"Maximum call stack size exceeded" with getter/sortBy (with Octane preview blueprint)
5 participants