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

Add measure hook #11506

Closed
krisselden opened this issue Jun 18, 2015 · 15 comments
Closed

Add measure hook #11506

krisselden opened this issue Jun 18, 2015 · 15 comments
Labels

Comments

@krisselden
Copy link
Contributor

Add a measure hook that corresponds to didRender that silences deprecation notice for scheduling a revalidation during render.

Currently if you cause a revalidation of render during render you get a deprecation during rendering implying that support for this will eventually go away. There still exists a use case for this, where you need to render something different based on measurements of what has been rendered so far.

An example would be if you increase the height and wanted to add an additional subtree like list-view.

@mixonic
Copy link
Member

mixonic commented Jun 18, 2015

@krisselden why not just schedule the measure into actions from didRender? A new hook that causes the same performance penalty as current usage seems fairly arbitrary. At least kicking it to actions means you hopefully get a single rerender even if many components measure.

@stefanpenner
Copy link
Member

@krisselden why not just schedule the measure into actions from didRender

because other didRender hooks including this one, may actually schedule stuff on actions. This mix will result in interleavings we aim to avoid with the bucketed run-loop approach.

@krisselden
Copy link
Contributor Author

Most of the time, measure will not cause a schedule revalidate. @stefanpenner I think he means schedule the thing that would cause the invalidation. I think would work, I will try that. We only need it in the list view case, if we run out of reuse-able bins and need to add more, because a size estimate was not correct.

@stefanpenner
Copy link
Member

I think would work, I will try that.

  didRender(){
    Ember.run.scheduleOnce('actions', this, this.confirmDimensions)
  }

would result in, this.confirmDimensions interleaving with other (non render/template related work), I believe we actually want to avoid this. Consider the nested component case.

@mixonic
Copy link
Member

mixonic commented Jun 18, 2015

@stefanpenner can you unpack that?

other didRender hooks including this one, may actually schedule stuff on actions

Are you suggesting that measure will schedule automatically, or that it is important that there is no difference in the timing of measure and didRender?

If there is no difference in the timing, then you suggest the only difference is if the warning about poor performance is logged?

@stefanpenner
Copy link
Member

the cost should be minimal

function invokeDidRender() {
  if (this.measure) { this.measure(); }
  warnAgainstRevalidation(this, this.didRender);
}

@stefanpenner
Copy link
Member

alternatively, a power-user helper could exist.

didRender() {
  dontWantDepcrecation (function(){
    doThingThatMayChecksDimensionsAndMayCauseChanges();
  })
}

@mixonic
Copy link
Member

mixonic commented Jun 18, 2015

I'm foreseeing a warning like:

Setting state during `didRender` results in poor rendering performance. If
you require measurement of the DOM before setting state, please use `measure`.

At which point most devs just dump all the logic into measure blindly.

I guess I'm saying that I like the idea of somehow informing people that they have done something that might be bad, but that a second junk drawer to silence the message does not seem great to me.

@stefanpenner
Copy link
Member

I guess I'm saying that I like the idea of somehow informing people that they have done something that might be bad, but that a second junk drawer to silence the message does not seem great to me.

then it should be some power-user helper. The reality is, multi-pass layouts are a norm for UI development. Some-how we should expose something to allow for this, throwing something onto another queue, doesn't seem correct either.

@mixonic
Copy link
Member

mixonic commented Jun 18, 2015

I do not necessarily prefer the approach, but @eccegordo is advocating for us to handle this at the logging level instead: emberjs/rfcs#65 (comment) log the warning, but have some silencing system (I have no idea how you would persist that though).

@eccegordo
Copy link
Contributor

@mixonic At the end of the day I care mostly that whatever messages, warning, deprecations, etc are emitted from the system are reasonably actionable. And silencing the message is one potentially valid course of action.

There are of course different ways to tackle this problem generally.

I am just thinking about the case where we help relatively inexperienced devs effectively "divide and conquer" the problem if they are faced with a flood of warnings when they go to update their project.

@krisselden
Copy link
Contributor Author

@eccegordo you don't need to address every deprecation right away, deprecations are not breaking, they are just flagging things that will go away eventually.

@eccegordo
Copy link
Contributor

@krisselden understood. Actually I was trying to clear everything on 1.13 so I could immediately try 2.0.

Unfortunately, the inspector is/was broken so I couldn't use that interface to review the deprecations and so having to wade through a lot of noise being spewed in the console. Which was what prompted the desire to selectively silence some warnings.

Plus I have bad OCD 😄

@stefanpenner
Copy link
Member

@krisselden this should likely be an RFC, or an issue proposal to the RFC repo.

@locks
Copy link
Contributor

locks commented Apr 15, 2016

I have moved this to emberjs/rfcs#134 according to @stefanpenner's last comment. Please continue the discussion there, thank you everyone.

@locks locks closed this as completed Apr 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants