Adds a loader element

[wesalvaro]

This fixes a major bug my team discovered.
It collects all the packages into one load call which is extremely important right now.
Now that all the packages are loaded at once, we can use the new loader.
Now that we are using the new loader, we can use the new material chart types.

This also redoes events to be more Polymer-y and changes the getImageURI function to a getter.

[wesalvaro]

@ebidel Hey, Eric! Super important changes in here.

My team discovered a ridiculous bug where calling the old loader multiple times resulted in variations of the corechart package being loaded. The versions were incompatible so it broke ... a lot. I wrote this new loader element to handle loading packages all at once and it also consolidates events and data object creation. This leaves the chart element itself to focus on just handling the chart stuff. Also fixes some tests.

If you could take a look, that'd be great!

[ebidel]

Don't think we should use arrow functions yet. IE 10/11 and Safari < 10 will have issues.

[ebidel]

Going to hold off removing the rest until the es6 features are removed. I'd also like to better understand the bugs you encountered and the perf hit of loading all the packages upfront. It makes the element much less granular. Can you post some numbers?

[wesalvaro]

Sorry about the es6, we use that internally, so this code is mostly a copy/paste of the fix I made to address the bug. Thanks for the quick review. I wanted to run the design by you before I worked on it too much in case you had some major concerns.

It's actually not a performance hit but a performance win.
JS and CSS are now loaded in two calls and now extraneous packages are loaded.
i.e. If you only use a bar chart, then only corechart will still be loaded.
If you use a table, then corechart and table will be loaded in one call.

The bug we encountered may or may not be something that only internal users were subject to. In our case, we make heavy use of the google-chart element. We even use raw Google Chart library to draw controls and dashboards (see #91 for that enhancement). We were loading the the library multiple times to load corechart, table, timeline, controls, etc. All loads were made individually. Here in Japan, we saw no issues, however our American friends reported the dashboard being completely out of service. We tried throttling and other situational changes to reproduce but could not. The only way that we were able to see the issue was to use a California VPN. The problems were just as they described. The library code was quite hard to debug as the only exceptions logged were inside eval'd code.

We saw the following exceptions (where x was one of a few different incantations):

1. You called the draw() method with the wrong type of data rather than a DataTable or DataView
2. Uncaught ReferenceError: xX is not defined.
3. Uncaught ReferenceError: gvjs_xx is not defined.

Our hypothesis was that different versions of the code were being loaded for Japan and America, however, the first exception really stood out to me. The chart was most definitely, always being drawn with a DataView. No question.

I logged the data properties at the draw call for all of the charts on the page and noticed that some DataTable instances were from a constructor Foo and some where from a constructor Bar (where Foo and Bar were some compiled identifier names). Foo and Bar contained no overlapping compiled properties (xx vs yy for the same value) but both contained the same and correct public API methods. This led to use making some tweaks to call the library only once but with all the required packages. All the problems went away.

Thus, this element was born.

It consolidates all of the package loading into one global state. It debounces the actual load call to collect as many packages into the load as possible. The loader can be primed with any extra required packages, as well. It separates out the package logic from the chart logic meaning that users can create their own Google Visualization stuff that this element does not currently support. This also means we do not need to rely on the Google Visualization team to allow us to call load more than once (fixing that bug for good). Consolidating all of an app's visualization loading logic here also means more flexibility. We could alter this element to decrease the debounce time and/or make multiple/more intelligent calls later or change the version of the library loaded via a property.

The point is that this allows users much greater flexibility and stability. Completely eliminating the need to do their own library loads outside of using the google-chart element. Instead of doing a manual import of some version of the loader (new? old?) and then loading some version of the library itself (43? 1? 1.0? 1.1? 45? current?) and having them be incompatible, they can just drop the google-chart-loader element on the page and they're done. This problem is only compounded when multiple charts are used on a page.

Now, users can do:

<google-chart-loader
    prime='["controls"]' on-loaded="_createControl"></google-chart-loader>

or:

const loader = document.createElement('google-chart-loader');
const data = loader.dataTable(...);

instead of:

<script src="loader??.js"></script>
attached/ready: function() {
  google.load('visualization', {
    version: '?', packages: ['corechart', 'controls'], callback: () => { this._createControl(); }
  });
},

This also means that users will be able to do more Polymer-y event handling (although currently only implemented for the existing events):

listeners: [
  'google-chart-ready': '_chartReady',
  'google-chart-select': '_chartSelectionChanged',
];

I hope this clears things up, but please let me know if you have any other questions. I'll remove the es6 stuff, but in the mean time, please still give the code a design review when you can.

Thanks,
-Wes

[wesalvaro]

A note on performance:
As expected the debounce causes the load to take an extra 100 ms.

Time to first render of a chart using just the corechart package*:

• Without collective loading: 750-950ms
• With collective loading: 850-1050ms

Page load time for multiple chart types**:

• Without collective loading: 2.56-2.76s
• With collective loading: 2.88s

It seems the collective loading version spends more time idle. This indicates the 100ms may be too long, but I think it's still a good value to at least start with. We could include a property to have it load immediately in the case of known chart packages.

<google-chart-loader packages='["table", "timeline"]' delay="0"></google-chart-loader>

*Measured by "Default Functionality > fires google-chart-render event for initial load" test.

**Using the demo page. Not a completely fair test in this case sense the version with collective loading also loads and renders a material Bar chart and does not have the load() called multiple times errors thrown.

[wesalvaro]

@ebidel : All of your comments have been integrated. PTAL

[ebidel]

this should handle if google is undefined

[wesalvaro]

I'm not sure I follow what you mean.
This is only called after google and google.charts or google.visualization is loaded.

[ebidel]

Right, but returning a default ns would be good just in case.

if (!window.google) {
  // return default ns.
}

[wesalvaro]

I think we may be talking about different things.
In this case, the namespace is where the chart constructor is located.
For traditional chart types, that namespace is google.visualization.
For new (material) charts, that namespace is google.charts.

e.g.

google.visualization.BarChart // traditional bar chart
google.charts.Bar  // material bar/column chart

So there is no default that would work. The calling function would look for defaultns.BarChart or something which would be equally incorrect.

This function is called inside of a protected area where the required package is guaranteed to have already been loaded. There is no way to recover from a situation where the package is loaded but google or google.charts is not available.

The exception thrown Uncaught ReferenceError: google is not defined LGTM in this case. If we were to check if google exists, it would just be to throw a different exception.

What do you think?

[ebidel]

The refactoring seems 👍 Haven't tested it myself, but it's more modular and seems to clean up the codebase.

[wesalvaro]

Cool, I'll integrate these changes this weekend. I have a follow-up PR that removes all the loading based promises from inside the chart element since now we have Polymer events and bindings for the loading. Cleans up even more stuff.

[ebidel]

I think just the one comment left.

[wesalvaro]

Finished adding documentation.

[ebidel]

pkg: 'bar'

[ebidel]

nvm, I see others are like this already.

[ebidel]

LGTM

[Gopinathp]

When is this releasing? I am struck at using google chart's treemap using polymer. Please help.

[Gopinathp]

I have realised that it has already been released. Thanks.

[alanent]

Someone can help me for the integration in angular ? https://stackoverflow.com/q/51153920/9962015