check interaction with render modifiers

[lifeart]

// background - https://github.com/emberjs/ember.js/pull/18698#issuecomment-579991774 https://github.com/lifeart/ember-ref-modifier/blob/master/tests/dummy/app/components/glimmer-ref.js

since we setting property in next runloop, such code may fail

 <div {{ref this "menuElement"}} {{did-insert this.focusOnFirstItem}} >

in other side, callback may solve issue

<div {{ref this.setMenuElement}} >
<div {{did-insert this.setMenuElement}} >

@action
setMenuElement(node) {
  this.menuElement  = node;
  this.focusOnFirstItem(node);
}

in other way, we can make next runloop sheduling optional, or remove it at all from modifier itself and give control to developers.

{{ref this "prop"}} - for classic usage
{{ref this "prop" run="next"}}  - for runloop control
{{ref this.setElement run="next"}} - runloop controlled callback
{{ref (next-runloop this.setElement)}} - manual callback control

other way - allow tracked chain computations rerun in same transaction if some of tracked properties is DOM node, but it seems low-level change.

[lifeart]

@wycats, @pzuraq, @jrjohnson any ideas?

https://github.com/ilios/common/blob/af6581698e44e8af7d988234e54e813ee1a75a10/addon/components/session/publication-menu.hbs#L27

[jrjohnson]

Unfortunately because of the upstream assertion this is going to be difficult to use either way because {{ref may only stop working when some set of re-rendering conditions is met which may not exist when the modifier is first added to the page, but only later when it is used in a render calculation somewhere.

My personal opinion is that this modifier should not invoke run.next() by default, but should provide an option to use it when setting the value would modify the @tracked chain. I think the current behavior is too surprising and having this hidden asynchronous set makes for difficult to track down timing bugs. (I've just spent a few hours tracking one down in ember-popper),

If {{ref (next-runloop this.setElement)}} is possible, it's a nice syntax. I think because of the callback set being available already that {{ref this.setElement}} with a setter like

@action
setElement(node){
  //set 'prop' in the next runloop since it is needed in this.calculateSize and shouldn't cause a re-rendering calculation
  run.next(()=> {
    this.prop = node
  });
}

Is obviously more verbose, but fairly clear and readable and doable today with no changed.

[lifeart]

I agree with all conserns, but for ember newcomers tracking-down errors like it pretty hard case. And write run.next in each usage is pretty uncomfortable case. Personally, I would prefer run modifiers in another transaction at all, instead of at the end of rendering transaction cycle. Because there is no official way to interop with DOM nodes/system apis and tracked stack at all.

[jrjohnson]

I agree, that message is very difficult to debug. On the other hand running all modifier in a new transaction (or with run.next() causes timing issues that get no visible warning, they just fail sometimes. Given these two outcomes I think the default should be to run synchronously with clear documentation for dealing with this message when it comes up.

[pzuraq]

FWIW, using the runloop like this is a major codesmell and I highly recommend avoiding it unless absolutely necessary. This may point toward needing to write more modifiers directly, and the ref pattern may just not be an ideal way to write Ember apps.

Basically, every time you do run.next, you are opting-in to a second render pass. That is cheap enough thanks to autotracking, but its still far more expensive than necessary. There are usually ways to accomplish what you want to without multiple passes, and the recommendation would be to organize code like this.

Custom modifiers are currently painful to write, since they are all global. I think template imports will help a lot with this.

[lifeart]

@pzuraq is it make sence to complete autotracking frame before modifiers run?

[lifeart]

or run next without triggering rerender (it can be triggered if property marked as tracked)

[lifeart]

oor, way to check is property tracked or not (@pzuraq) if tracked - run in next runloop, if not - just assign (have no idea how check property tracking status)

function ref(node, [  context, key ]) {
   if (isTracked(context, key)) {
     run.next(()=>context[key] = node);
   } else {
     context[key] = node;
  }
}

[pzuraq]

I don't think we will be exposing that functionality. Unidirectional dataflow is a good thing, not a bug to work around.

Edit: Moreover, it doesn't scale to other forms of root state. What if tracking is happening through a TrackedMap or TrackedArray?