removes the angular dependency from tag cloud #15779

thomasneirynck · 2017-12-27T20:38:32Z

Removes the angular dependency from tag cloud.

kobelb · 2017-12-28T20:17:21Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+  constructor(node, vis) {
+    this._containerNode = node;
+
+    const nodeContens = document.createElement('div');


nit: nodeContents is misspelled

kobelb · 2017-12-28T20:30:24Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+      this._resize();
+    }
+
+    await this._renderComplete$.take(1).toPromise();


Observables don't start "listening" until they are subscribed to, which is being done by the .toPromise call. If the renderComplete event was fired before we got to this line, then this isn't going to do what we want.

For each render() call, we're only interested in the first render-complete-call starting after line 59. Do we need to subscribe to that observable up front, or at the start of the method, or in the constructor when we create it?

If the renderComplete is only (and always) called after line 59 then the current approach should work.

thomasneirynck · 2018-01-05T13:52:52Z

@ppisljar still failing test, but could benefit from first look already

ppisljar

some small comments, but i really like the fact we are getting rid of angular! :)

ppisljar · 2018-01-09T11:02:43Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+
+    const nodeContens = document.createElement('div');
+    nodeContens.innerHTML = `
+            <div class="tagcloud-vis">


extract into html file and import it ?

ppisljar · 2018-01-09T11:03:43Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+      if (!this._bucketAgg) {
+        return;
+      }
+      const aggConfigResult = new AggConfigResult(this._bucketAgg, false, event, event);


this is now changed in master to use the bucket.createFilter and API.addFilters

ppisljar · 2018-01-09T11:06:13Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+    const truncatedMessage = this._containerNode.querySelector('.tagcloud-truncated-message');
+    const incompleteMessage = this._containerNode.querySelector('.tagcloud-incomplete-message');
+
+    if (!this._vis.aggs[0] || !this._vis.aggs[1]) {


extract this condition to a new variable hasAggsDefined so its easier to read ?

ppisljar · 2018-01-09T11:07:53Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+    }
+
+    const data = response.tables[0];
+    this._bucketAgg = data.columns[0].aggConfig;


get the agg from vis.aggs instead of data ?

thomasneirynck · 2018-01-19T16:06:49Z

rebasing

ppisljar · 2018-01-23T15:52:44Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

+    }
+
+    const bucketName = this._containerNode.querySelector('.tagcloud-custom-label');
+    bucketName.innerHTML = `${this._vis.aggs[0].makeLabel()} - ${this._vis.aggs[1].makeLabel()}`;


use textContent instead of innerHTML

thomasneirynck · 2018-01-25T16:27:33Z

jenkins, test this

kobelb · 2018-01-25T18:03:20Z

src/core_plugins/tagcloud/public/tag_cloud_visualization.js

@@ -0,0 +1,121 @@
+import TagCloud from './tag_cloud';
+import tagCloudContainer from './tag_cloud_container.html';


What made you choose to manually specify the container's html that is then modified using DOM methods/attributes as opposed to using React? This seems like it'd be a lot easier if we used React, and then we wouldn't have to manually ensure that we weren't opening ourselves up to XSS by ensure we're using the correct attributes on the elements themselves, and it'd be done automatically for us.

Couple reasons imho:

What would React offer beyond adding another abstraction? What's the attack vector here that React would solve? The template is static content.

The async nature of the visualization is wrapped cleanly in the async .render() call instead of wrapping this in a React component and then having to work around it.

We recommend that community users use the createBaseVisualization since it is the most straightforward one (and the most simple interface to maintain). I think we should eat our own dogfood in a couple of places, here as good a place as any.

i think we should change that suggestion to use createBaseVisualization with kibana 6.3 and suggest to use createReactVisualization. and limit usage of createBaseVisualization only for when visualization is based on some other rendering library (like d3 or leaflet).

^ very much disagree :)

What would React offer beyond adding another abstraction? What's the attack vector here that React would solve? The template is static content.

We're using node.textContent appropriately below when setting the label, but if we were to set innerHtml we open ourselves up to a potential XSS vulnerability (based on where the data comes from).

Additionally, we're dynamically setting styling throughout based on parameters when React provides a clear way to do so.

dangerouslySetInnerHtml is just as dangerous :)

i think with tagcloud (if going react way) we would need to rewrite whole tagcloud library. If the goal is getting rid of angular i think this current approach is good enough.

I think we should not at all impose a rendering-technology on our community developers. This almost as a matter of principle. We are not creating flexible interfaces when users need to follow the implementation-detail of the day. It used to be angular in Kibana, now it is React. Those are decisions that make sense for us (enforcing consistent tooling, enforcing consistent L&F ...), but have much less relevance for outside users.

Also, we need to really determine what the added value React brings as a wrapper for a Visualization-plugin. IMO this is very little for 80% of the visualizations. A visualization, almost by definition, is going to be not something textual or vanilla DOM-like and will rely on some other visualization library, that will introduce SVG, canvas, GL, clever use of HTML, ... What is React offering beyond a templating engine for that? Visualizations also should be a lot more asynchronous and stateful, so we can make efficient use of the DOM and animations. React doesn't really help with that, because those implementation details of how to do that efficiently will depend very much from visualization to visualization.

btw: not saying we shouldn't rewrite this in react, just justificating my LGTM :)

dangerouslySetInnerHtml is just as dangerous :)

Agreed, but we wouldn't need that here. We could grab the ref and pass it to the TagCloud and let it do it's thing. It doesn't mean that the TagCloud itself couldn't do some nasty stuff, but at least the code that we currently "own" would get the default security mechanisms.

Also, we need to really determine what the added value React brings as a wrapper for a Visualization-plugin.

As soon as we end up straying from standard HTML elements, I agree with you. However, in this situation a templating language for HTML is exactly what we need.

I'm all for us getting rid of AngularJS, but I'm not following why we're doing all this manually in this situation. Using React here would be less code, more consistent with the current direction and more secure by default.

My comment regarding the discussion can be found in the general PR comments :-)

thomasneirynck · 2018-01-25T20:51:27Z

hey @timroes, can you review too? need decision on whether we should use React here or not. thx!

timroes · 2018-01-26T11:18:54Z

Before reviewing this, I also want to share my 2 cents regarding the React discussion.

I think we shouldn't put any recommendations on the community, what they are using, so they can use base, React, etc. whatever they want - maybe we can get rid of the Angular one at some point, since the digest cycle is really causing troubles.

Regarding what we should use and why to build the templates: I would totally agree with Brandon here, and think we gain a lot of benefit of using React also for some smaller things like that.
Of course you can also use dangerouslySetInnerHtml in React, but it's way more unlikely that you do it by accident, than using innerHTML and "accidentally" insert user content:

container.innerHTML = `<div class="foo">
  <div class="content">${vis.name}</div>
</div>

That example above can accidentally happen rather early. If you would use React, you would usually have written that as:

return (<div className="foo">
  <div className="content">{ vis.name }</div>
</div>);

and everything would be fine. Of course you can also create the same problem if you rewrite it to:

return (<div className="foo">
  <div className="content" dangerouslySetInnerHtml={vis.name}></div>
</div>);

But it's way more unlikely that this will happen "by accident" as the above one.

Also as soon as we have something like a loop of element or an condition, we usually already gain a lot of benefits from using React. For example if we look at the metrics visualization. By using React there, we suddenly don't clear the container on every render, and create elements within a loop again, but by using React there for the loop and the rendering, we basically just update the DOM with the stuff that actually changes... even if our render method is called way more often (which in our current infrastructure is the case).

So by changing Metrics and Markdown Vis to React, we basically got from updating the DOM nearly every render approach with a handful of DOM nodes, to basically no update if it's not absolutely required. Also let's not talk about, how imperformant innerHTML actually is, in contrast to using the proper appendChild, etc. nodes.

So my rule of thumb would be (for our visualizations, whether or not we recommend that to the community, I don't mind atm): As long as we need to create more than 1 or 2 elements, that need to be nested together, and for sure as soon as we have conditions or loops in there, let's use React for it. We will be safer and way more performant. If we just attach one canvas element and draw in this, or just one DOM node, that we pass to d3 (or something else), I am fine using Vanilla JS for it.

So regarding this PR and the different error messages, I would second Brandon's opinion and would rather use React for that, since this would make line 50 to 76 imho way better readable and thus maintainable, and also more performant, since we wouldn't need to use querySelector anymore.

thomasneirynck · 2018-01-26T14:50:32Z

@timroes @kobelb @ppisljar understood! I'll change this to React. 🙇

elasticmachine · 2018-01-31T01:45:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

add unit tests for Visualization interface

This reverts commit ada0c89.

This reverts commit c7f6691.

thomasneirynck · 2018-03-13T19:58:26Z

rebasing with master broke this PR. please discard latest.

elasticmachine · 2018-03-13T20:25:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

thomasneirynck · 2018-03-15T15:37:02Z

reverting this back to the react way of setting the label and feedback message. linting rules prevent setting of .innerHTML directly.

elasticmachine · 2018-03-15T16:13:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

thomasneirynck · 2018-03-15T18:04:40Z

jenkins, test this

elasticmachine · 2018-03-15T18:49:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

- Implements tag_cloud as a Base Visualization - Use React for labels - Introduce screenshot comparison unit tests

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) chore labels Dec 27, 2017

thomasneirynck force-pushed the chore/remove_angular_tagcloud branch from a9c669e to cf280b2 Compare December 28, 2017 19:54

thomasneirynck requested a review from kobelb December 28, 2017 19:59

kobelb reviewed Dec 28, 2017

View reviewed changes

thomasneirynck requested a review from ppisljar January 5, 2018 13:52

ppisljar requested changes Jan 9, 2018

View reviewed changes

thomasneirynck force-pushed the chore/remove_angular_tagcloud branch from cf280b2 to 1eb7214 Compare January 19, 2018 16:06

thomasneirynck force-pushed the chore/remove_angular_tagcloud branch from 1fc584e to 61c6a7e Compare January 23, 2018 01:10

ppisljar reviewed Jan 23, 2018

View reviewed changes

thomasneirynck force-pushed the chore/remove_angular_tagcloud branch from 71ad1bf to 5de2935 Compare January 23, 2018 19:25

thomasneirynck added v7.0.0 v6.3.0 labels Jan 23, 2018

kobelb reviewed Jan 25, 2018

View reviewed changes

ppisljar approved these changes Jan 25, 2018

View reviewed changes

thomasneirynck requested a review from timroes January 25, 2018 20:50

thomasneirynck force-pushed the chore/remove_angular_tagcloud branch from c7c9ff1 to 036c277 Compare January 31, 2018 00:59

thomasneirynck mentioned this pull request Feb 5, 2018

Remove flaky test #16534

Merged

timroes mentioned this pull request Feb 14, 2018

[Meta] Reactification/EUIfication of visualizations code #16728

Closed

13 tasks

thomasneirynck mentioned this pull request Mar 3, 2018

updating tagcloud to react #16569

Closed

thomasneirynck added 3 commits March 13, 2018 15:44

removes the angular dependency from tag_cloud_visualization

a1fe644

add unit tests for Visualization interface

fix mousehovers

6912ccc

relax threshold

93e94fc

thomasneirynck added 4 commits March 13, 2018 15:44

review feedback

67f169f

extract template

246d284

wrap notificaitons in React component

c7f6691

remove setting of vars

ada0c89

thomasneirynck force-pushed the chore/remove_angular_tagcloud branch from 2c71f1a to ada0c89 Compare March 13, 2018 19:45

thomasneirynck added 2 commits March 13, 2018 15:47

Revert "remove setting of vars"

1f21dd9

This reverts commit ada0c89.

Revert "wrap notificaitons in React component"

12a941f

This reverts commit c7f6691.

thomasneirynck added 3 commits March 15, 2018 10:38

listen to data changes

b9b1615

wrap notificaitons in React component

e1a2695

remove setting of vars

85e3c15

timroes approved these changes Mar 15, 2018

View reviewed changes

thomasneirynck merged commit a417b11 into elastic:master Mar 15, 2018

thomasneirynck mentioned this pull request Mar 15, 2018

[6.x] Removes the angular dependency from tag cloud (#15779) #17182

Merged

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Mar 15, 2018

Removes the angular dependency from tag cloud (elastic#15779)

2265d37

- Implements tag_cloud as a Base Visualization - Use React for labels - Introduce screenshot comparison unit tests

thomasneirynck added a commit that referenced this pull request Mar 15, 2018

Removes the angular dependency from tag cloud (#15779) (#17182)

46641f9

- Implements tag_cloud as a Base Visualization - Use React for labels - Introduce screenshot comparison unit tests

thomasneirynck mentioned this pull request Mar 16, 2018

Flaky tag cloud visualization test #16383

Closed

thomasneirynck mentioned this pull request Apr 6, 2018

Replace reference screenshots #17597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removes the angular dependency from tag cloud #15779

removes the angular dependency from tag cloud #15779

thomasneirynck commented Dec 27, 2017 •

edited

Loading

kobelb Dec 28, 2017

kobelb Dec 28, 2017

thomasneirynck Dec 28, 2017

kobelb Dec 29, 2017 •

edited

Loading

thomasneirynck commented Jan 5, 2018

ppisljar left a comment

ppisljar Jan 9, 2018

ppisljar Jan 9, 2018

ppisljar Jan 9, 2018

ppisljar Jan 9, 2018

thomasneirynck commented Jan 19, 2018

ppisljar Jan 23, 2018

thomasneirynck commented Jan 25, 2018

kobelb Jan 25, 2018

thomasneirynck Jan 25, 2018

ppisljar Jan 25, 2018

thomasneirynck Jan 25, 2018

kobelb Jan 25, 2018

ppisljar Jan 25, 2018

thomasneirynck Jan 25, 2018

ppisljar Jan 25, 2018

kobelb Jan 25, 2018

timroes Jan 26, 2018

thomasneirynck commented Jan 25, 2018

timroes commented Jan 26, 2018

thomasneirynck commented Jan 26, 2018

elasticmachine commented Jan 31, 2018

thomasneirynck commented Mar 13, 2018

elasticmachine commented Mar 13, 2018

thomasneirynck commented Mar 15, 2018

elasticmachine commented Mar 15, 2018

thomasneirynck commented Mar 15, 2018

elasticmachine commented Mar 15, 2018

		@@ -0,0 +1,121 @@
		import TagCloud from './tag_cloud';
		import tagCloudContainer from './tag_cloud_container.html';

removes the angular dependency from tag cloud #15779

removes the angular dependency from tag cloud #15779

Conversation

thomasneirynck commented Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

thomasneirynck commented Jan 5, 2018

ppisljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Jan 19, 2018

Choose a reason for hiding this comment

thomasneirynck commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Jan 25, 2018

timroes commented Jan 26, 2018

thomasneirynck commented Jan 26, 2018

elasticmachine commented Jan 31, 2018

💚 Build Succeeded

thomasneirynck commented Mar 13, 2018

elasticmachine commented Mar 13, 2018

💔 Build Failed

thomasneirynck commented Mar 15, 2018

elasticmachine commented Mar 15, 2018

💚 Build Succeeded

thomasneirynck commented Mar 15, 2018

elasticmachine commented Mar 15, 2018

💚 Build Succeeded

thomasneirynck commented Dec 27, 2017 •

edited

Loading

kobelb Dec 29, 2017 •

edited

Loading