engageLively / galyleo-dashboard

3 stars 2 forks source link

Multiple Issues when running inside JupyterLab #110

Closed rickmcgeer closed 1 year ago

rickmcgeer commented 1 year ago
  1. Sometimes the Google charts package doesn't load $world.get('dashboard').viewModel.gViz is undefined
  2. When it does load, the charts are properly loaded in the tables but are not drawn
  3. await $world.get('dashboard').viewModel.drawAllCharts() does draw the charts, but they are all at position (0, 0)
  4. When I move the charts by hand (Halo still works) I can see they're all there and everything works fine.
  5. This doesn't happen when I'm looking at the frozen dashboard standalone https://matt.engagelively.com/users/rick/published/studio/index.html. It's only when I instantiate inside JupyterLab
  6. I'm mystified. Assigning to @merryman to see if he has any ideas.
  7. This may have started happening when (at @linusha's suggestion, to fix a different bug) I set renderViaCSS on top bar to false. Will reset to true to check this.
merryman commented 1 year ago

I think the first point already is an untenable situation. It can't be that we need to essentially busy wait on a global variable to be populated. There needs to be a more canonical way to ensure that google charts is loaded, or some kind of promise we can reliably wait on.

edit: Appears like google killed Support for charts two months ago: https://github.com/google/charts/issues/798. Tbh. I don't know how much more work we should be putting towards google charts anymore.

rickmcgeer commented 1 year ago

I'd love to dump Google Charts. FTM, since the project is no longer maintained, the simple solution is for Google to open-source it and let us download the damn thing through NPM, like any other package. That said, the issue is doing a chart editor. I don't think it's that hard, a month, tops. But I don't want to wait a month to do the new release. So I'd like us to put in what I hope is a couple days' effort to get this into a maintainable, shippable state, do the release, then work damned hard on the chart editor and cut over to Chartjs/Leaflet/Cytoscape.

rickmcgeer commented 1 year ago

Incidentally, I set renderViaCSS to true and the problem continues. The next experiment is to try this in a vanilla iframe to see if that is the problem. My guess is that it is due to some strange effect with the Jupyter proxy... Note that even when Google charts loads:

So my guess is that there is a timing issue that is triggered by the multiple timeouts, and as a result a ton of async code isn't getting called. I'll put in a test for those next.

rickmcgeer commented 1 year ago

OK, I think I have a lead. I put in console.log messages when the morphic properties for a chart are assigned, and when the chart is first drawn. They didn't appear, and a message saying gViz is null DID appear. When I checked gViz in the console, it was there, and then I reloaded the dashboard using $world.get('dashboard').viewModel.loadTestDashboard('drilldown-test') and the messages appeared and the chart drew properly. So I think the fix is to wait for gViz to load... Goddamned Google. They're the MS of the 2020s.

rickmcgeer commented 1 year ago

And, @merryman, you're right. We have to ditch Google Charts asap. I'm going to devote fulltime to the editor. It's the only way.

rickmcgeer commented 1 year ago

I got it fixed, though frankly the fix shouldn't have been necessary. I put in a defensive call in addChart

if (this.gViz) {
    this.drawChart(chartName);
}

which shouldn't be necessary, because loadGoogleChartPackages is in init, it's async and we wait for it, and this happens (and logging confirms it) BEFORE we attempt to draw any charts.

Since we may have not drawn charts because Google Charts wasn't loaded, a callback was added to the charts loader: (_ => this.drawAllCharts();

rickmcgeer commented 1 year ago

I'm leaving this issue open because we need to talk about it. Mostly, this is Google stabbing us in the back, @merryman please look at the code and see if there is anything I have missed. Here is the init code:

$world.execCommand("open browser", {moduleName: "studio/dashboard.cp.js", packageName: "galyleo-dashboard", codeEntity: [{"name":"Dashboard","type":"class-decl"},{"name":"init","type":"class-instance-method"}]});

And addChart:

$world.execCommand("open browser", {moduleName: "studio/dashboard.cp.js", packageName: "galyleo-dashboard", codeEntity: [{"name":"Dashboard","type":"class-decl"},{"name":"removeChart","type":"class-instance-method"}]});

Everything checked in.

rickmcgeer commented 1 year ago

Fixed and closed