Eliminate circular dependencies #2032

rreusser · 2017-09-23T00:36:33Z

resolves #236.

This PR eliminates all circular dependencies (🎉), mainly by adding gd._plotAPI.* (as suggested in #236) which binds (curries?) the gd to the arguments automatically. That is, you can call gd._plotAPI.redraw() or gd._plotAPI.restyle([....]). This eliminates the need for var Plotly = require('../../plotly'), which accounted for the vast majority of circular dependencies.

I didn't really find a need for nontrivial changes (has the component registry been implemented since #236?) since all other circular dependencies could be resolved by moving one or two functions to their own files. Though it's a bit ugly to require get_subplot_id.js separately. I'm open to different thoughts on this.

I expect a couple failing tests to remain since spyOn got somewhat more tricky to enforce and required binding the API manually in order to make sure it was the correct, spied-upon API.

Edit: almost. One circular dependency remains. Remaining issue:

toImage requires plot api --> circular

rreusser · 2017-09-23T00:41:48Z

src/plots/get_subplot_ids.js

+ * @return {array} list of ordered subplot ids (strings).
+ *
+ */
+module.exports = function getSubplotIds(layout, type) {


I don't like this. Moving this one file broke a long circular chain in which Plots was required by plots/cartesian/axis_ids.js.

rreusser · 2017-09-23T00:43:44Z

src/plots/font_weight.js

+'use strict';
+
+// TODO make this a plot attribute?
+module.exports.fontWeight = 'normal';


This seems silly to have in its own file but was another one of those couple things that required a giant circular require of top-level Plots by some deeply nested component.

This is only used in one place, titles - everywhere else we just implicitly inherit font-weight: normal.

One can set font-weight: bold or something on gd or a parent, and have that inherited by text inside the plot. We could argue about whether that's a reasonable thing to allow (in particular, exports would NOT inherit this bolding) but it seems silly to ONLY explicitly remove this from the title. I'd 🔪 this file AND the place it's used in titles/index.js. Later if we decide this is important we can apply normal weight to our whole <svg> elements or something.

By the way, that font weight key has to be exposed on Plots for this.

By the way, that font weight key has to be exposed on Plots for this.

Ah, crazy - as it stands now this PR will break that, but now I get why this was in there in the first place. So it's really just to allow us to make titles bold by default. Then lets have this an attribute of the Titles object, ie Titles.fontWeight, so it can be overridden by saying var Titles = require('./components/titles'); Titles.fontWeight = 900;

rreusser · 2017-09-23T00:44:33Z

src/plots/plots.js

@@ -1865,7 +1811,7 @@ plots.extendTrace = function(destTrace, srcTrace) {
 * The result is the original object reference with the new contents merged in.
 */
 plots.extendLayout = function(destLayout, srcLayout) {
-    return plots.extendObjectWithContainers(destLayout, srcLayout, plots.layoutArrayContainers);
+    return plots.extendObjectWithContainers(destLayout, srcLayout, Registry.layoutArrayContainers);


plots vs. Registry was aliased but not updated in quite a few places. It should be otherwise equivalent.

rreusser · 2017-09-23T00:45:07Z

test/jasmine/tests/lib_test.js

@@ -2008,59 +2008,59 @@ describe('Queue', function() {
        })
        .then(function() {
            expect(gd.undoQueue.index).toEqual(1);
-            expect(gd.undoQueue.queue[0].undo.args[0][1]['marker.color']).toEqual([null]);
-            expect(gd.undoQueue.queue[0].redo.args[0][1]['marker.color']).toEqual('red');
+            expect(gd.undoQueue.queue[0].undo.args[0][0]['marker.color']).toEqual([null]);


With gd gone from the first argument, these all shifted over one.

etpinard · 2017-09-25T19:08:32Z

tasks/test_syntax.js

@@ -195,7 +195,7 @@ function assertCircularDeps() {
        var logs = [];

        // see https://github.com/plotly/plotly.js/milestone/9
-        var MAX_ALLOWED_CIRCULAR_DEPS = 17;
+        var MAX_ALLOWED_CIRCULAR_DEPS = 0;


Welll… 1. In order to actually get all tests passing. 😬

rreusser · 2017-09-26T17:22:46Z

I'm going to put this on ice for just a moment since I got a bit tripped up trying to get rid of the last circular dependency. It suggests moving toward a register approach is probably the right answer, but it gets a little treacherous to keep things testable (spies, in particular) and to avoid introducing any extra circularity. It's proved very doable, but will require just a bit of finesse to really nail it.

rreusser · 2017-10-12T14:23:53Z

Unchecked memory usage in firefox… I wonder if this is also related to circular dependencies: plotly/plotly.R#483

etpinard · 2017-11-17T16:24:58Z

Closing this thing - as this was put on ice ⛸️

Refer to https://github.com/plotly/plotly.js/milestone/9 for more on the topic

Eliminiate circular dependencies

9a45c5c

rreusser added type: maintenance status: has TODOs labels Sep 23, 2017

rreusser commented Sep 23, 2017

View reviewed changes

rreusser added 3 commits September 22, 2017 17:53

Fix commmand test spy

6155d55

Delete a line to re-trigger circle-ci

8c09468

Fix _plotAPI typo

ec1bf51

rreusser changed the title ~~Eliminiate circular dependencies~~ Eliminate circular dependencies Sep 23, 2017

Fix plots test

d9a3df8

etpinard reviewed Sep 25, 2017

View reviewed changes

rreusser added status: on hold and removed status: has TODOs labels Sep 26, 2017

etpinard added this to the Decircularise src dependencies milestone Nov 17, 2017

etpinard closed this Nov 17, 2017

etpinard deleted the eliminate-circularity branch November 17, 2017 16:25

etpinard mentioned this pull request Nov 17, 2017

De-circularize plotly.js require statements #236

Closed

etpinard mentioned this pull request Mar 1, 2018

ZERO circular dependency #2429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate circular dependencies #2032

Eliminate circular dependencies #2032

rreusser commented Sep 23, 2017 •

edited by etpinard

Loading

rreusser Sep 23, 2017 •

edited

Loading

rreusser Sep 23, 2017

alexcjohnson Sep 25, 2017

etpinard Sep 25, 2017

alexcjohnson Sep 26, 2017

rreusser Sep 23, 2017

rreusser Sep 23, 2017

etpinard Sep 25, 2017

rreusser Sep 25, 2017

rreusser commented Sep 26, 2017

rreusser commented Oct 12, 2017 •

edited

Loading

etpinard commented Nov 17, 2017

Eliminate circular dependencies #2032

Eliminate circular dependencies #2032

Conversation

rreusser commented Sep 23, 2017 • edited by etpinard Loading

rreusser Sep 23, 2017 • edited Loading

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

rreusser commented Sep 26, 2017

rreusser commented Oct 12, 2017 • edited Loading

etpinard commented Nov 17, 2017

rreusser commented Sep 23, 2017 •

edited by etpinard

Loading

rreusser Sep 23, 2017 •

edited

Loading

rreusser commented Oct 12, 2017 •

edited

Loading