Consider separating `triggerUpdate` from `chartData`

[jacobq]

Of course, it'd be nice to eliminate the need for triggerUpdate at all, but in lieu of a better solution, this flag should probably at least be moved into its own separate attribute....This needs some thought though. On one hand, chartData may be a computed property, in which case it'd be a little weird to incorporate another dependent key into its calculation to toggle this, and it'd be bad to mess with the property after its been computed (violates 'pure function' assumption of CPs). On the other hand, the part of the code that's generating the data is the part that's most likely to be signaling that an update is needed, and it may only have a reference to the chartData object...

ember-cli-plotly/addon/components/plot-ly.js

Line 94 in b891ff4

_triggerUpdate: observer('chartData.triggerUpdate', function() {

[lolmaus]

Hey @jacobq, thank you for the addon and please excuse my tone below. 😇

---

How am I even supposed to use this?

I tried this:

  @className isFullScreen = false;
  triggerUpdate = false;


  @computed('data', 'triggerUpdate')
  get chartData(): Data[] {
    const chartData = this.data.series.map(/*...*/ );

    // @ts-ignore
    chartData.triggerUpdate = this.triggerUpdate;

    return chartData;
  }

  @observes('isFullScreen')
  triggerUpdateOnFullScreenChange() {
    next(this, () => this.toggleProperty('triggerUpdate'));
  }

It only starts working after resizing the window. Before that it's ignored.

And why the actual funk is the triggerUpdate property on an array? That makes no sense and causes TypeScript to freak out.

---

OK, trying to be cool now.

A proper way of passing actions down is to use an evented object.

In a parent component:

@computed
get eventBus () {
  return EmberObject.extend(Evented);
}

@action
toggleFullScreen() {
  this.eventBus.trigger('toggleFullScreen');
}

<PlotLy
  @eventBus={{this.eventBus}}
>

<button {{action 'toggleFullScreen'}}>
 Toggle full screen
</button>

In the plot-ly component:

@action
_triggerUpdate() {
  log(`_triggerUpdate observer firing`);
  scheduleOnce('render', this, '_react');
}

didInsertElement() {
  super.didInsertElement();
  this.eventBus.on('toggleFullScreen', this, this._triggerUpdate);
}

willDestroyElement() {
  super.willDestroyElement();
  this.eventBus.off('toggleFullScreen', this, this._triggerUpdate);
}

I would like to make a PR with the above.

@jacobq Are you OK with this approach?

[jacobq]

Hey @jacobq, thank you for the addon and please excuse my tone below. 😇

Haha, no worries. Sorry it's not in a great state right now. (In fairness, I did try to communicate this via the v0.x versioning.) Thanks for the feedback -- it encourages me to make time to improve this (otherwise I sometimes I assume I'm the only one). @wojm has kindly offered to collaborate, and I hope we'll be able to make some improvements in the near future. I just haven't had time to put into thinking about this (busy at work, have two children under two, <insert more excuses here>...)

How am I even supposed to use this?

triggerUpdate is a vestigial artifact that is (hopefully) going to be completely removed soon. Thus you really shouldn't use it at all. What I am doing instead right now (and this may not be optimal, but it seems to at least work in my case) is to use notifyPropertyChange:

series = [{
  x: [],
  y: []
}];

// x & y are arrays so this CP actually doesn't fire when their contents change
@computed('series.@each.{x,y}') 
get chartData() {
    // ...
}

// later when the underlying data changes
context.notifyPropertyChange('series');

I tried this:

You were on the right track with this.toggleProperty('triggerUpdate') in that that's how I was originally using it, though I was calling it when the data changed not when the view changed. Is the problem you're trying to solve related to the plot having out-of-date content or its appearance / size?

And why the actual funk is the triggerUpdate property on an array? That makes no sense and causes TypeScript to freak out.

If you explain to TypeScript that it's not just an array then it won't freak out.
The reason (good or bad) that it was put there was because it is only related to the data. It's job was to give the addon a nudge when the data has changed without the component otherwise easily being able to tell so that it can call plotly.js's API to update the output to match.
Admittedly, it's a bad approach to solving the problem, and it is going away.

A proper way of passing actions down is to use an evented object.

I appreciate the code you provided and will take a closer look at this idea later. Would you mind helping me understand the advantages of doing something like that vs. notifyPropertyChange? (and again, I am assuming the problem is stale data but maybe that's not the problem in your case)

I would like to make a PR with the above.
@jacobq Are you OK with this approach?

I'm sure it's certainly better than what I have now :/
However, I would like to avoid using EmberObject and also follow the DDAU convention / avoid sending events down.
Would you be offended if I merged the PR but then later replaced it with something else? If not then feel free to make a PR. Maybe some other developers will weigh in with idea too.

[lolmaus]

My use case is toggling full screen for a chart. I'm stretching the chart container to the viewport, but the chart stays small. I need to tell it to realign.

It has nothing to do with data which stays the same.

This case is sending an event down the DOM, whether you want it or not. Yes, it does not fit the DDAU paradigm, but it's a legit case that've ran into multiple times. DDAU does not cover everything.

notifyPropertyChange is fine for this particular case. The only benefit of my Evented approach over notifyPropertyChange here would be that notifyPropertyChange is obscure. It's a hack. Evented is straightforward & obvious (you need to pass an event down the DOM -- you do exactly that) at the cost of code being much more verbose (both in the addon and in the userland).

Most cases though require passing some options with the event, in which case notifyPropertyChange would not be enough. I have tried multiple approaches for this case and found ad-hoc Evented to be the best.

I've tried notifyPropertyChange as you suggest. It works, but it has the same problem as my triggerUpdate above: it only starts working after I resize a page once. Before resizing, it is ignored.

I guess that would happen to my Evented suggestion as well. I've debugged it a bit and learned that Plotly.react does get called on notifyPropertyChange, but does not result in realignment.

Do you know why?

[lolmaus]

Using window.dispatchEvent(new Event('resize')) instead of notifyPropertyChange works fine...