ZERO circular dependency #2429

etpinard · 2018-03-01T21:24:33Z

I started working on this PR when I noticed that the new layout grid module from #2399 has a circular dependency:

This went unnoticed because some other PR(s) removed one or two circular dependencies. On master we now have:

whereas our test-syntax script was happy with 16 or less.

So after fixing the grid circular dep (that's in commit bee37dd), I tried to see what it would take to 🔪 all circular dependencies. In brief, we had:

to register errorbars as a component and use Registry.getComponentMethod when needed (commit 87275af),
register the top-level api methods (i.e. the Plotly[method] functions) as first proposed in the last serious attempt Eliminate circular dependencies #2032 (comment)

and to my surprise this worked as a charm 🍀 🍾 🎉

Looking forward, removing these circular deps isn't just nice, it should allow us to trim our bundle size using browser-pack-flat or even tinyify. Preliminary attempts were positive: I think we'll be able to trim tens of kBs out of our bundles. I'll open up a new PR for this soon. Moreover, there were reports of some dev tools (the new FF ones I think) balking on our source code, this might help.

@alexcjohnson @nicolaskruchten hopefully you'll enjoy this. 😛

... so that its attributes are added to the schema free of circular deps.

- no need to have a separate method just for the top-level method.

... by using utilizing component-to-schema logic, instead of punching the error bar attributes into the trace declarations.

... so that down the road errorbars could be taken out or src/core.js, and be its own registrable module.

- export api methods in plot_api/index.js - expose and register them in src/core.js

nicolaskruchten · 2018-03-01T21:30:41Z

Anything that makes bundling easier, faster, smaller :)

alexcjohnson · 2018-03-01T22:43:33Z

src/plots/plots.js

 var Lib = require('../lib');
 var _ = Lib._;
 var Color = require('../components/color');
+var Grid = require('../components/grid');


Do you want to go the last inch here and turn Grid.method into Registry.getComponentMethod? Then grid becomes an optional component, right?

done in -> 9246fda

alexcjohnson · 2018-03-01T23:00:51Z

test/jasmine/tests/register_test.js

+        }, 'degauss')).toEqual([]);
+    });
+
+    it('returns an empty array if none present', function() {


test case description copied from above

But also, if you leave these un-nested, the descriptions should reference the method being tested.

done in e05f4f0

alexcjohnson · 2018-03-01T23:06:54Z

test/jasmine/tests/register_test.js

+
+    beforeAll(function() {
+        Registry.register(fakeModule);
+    });


do we need to remove this in afterAll (here and in the next suite), as we do in the suite above?

Good catch. Fixed in e05f4f0

alexcjohnson · 2018-03-01T23:41:07Z

src/registry.js

 */
+exports.call = function(name, args) {
+    return exports.apiMethodRegistry[name].apply(null, args);
+};


Can we get rid of the array wrapping the args, like other call methods (d3.select().call and native function.call)? Borrowing from d3 and condensing, seems like it would just be:

return exports.apiMethodRegistry[name].apply(null, [].slice.call(arguments, 1));

or, perhaps marginally faster if we don't care about this being the method name:

return exports.apiMethodRegistry[name].apply([].slice.call(arguments));

That's what I was going to do until I saw this block (called from updatemenus and silders) where the argument list is built as an array.

I think I would prefer keeping this pattern. I could rename Registry.call -> Registry.apply to avoid confusion on the call signature if you prefer.

Ah right, that's the one place you use Registry.call without brackets, and it would be awkward to try and fit that into the call pattern. It's a shame that this one call adds cruft to the hundred other ones though. What about creating both Registry.call and Registry.apply methods? Or, since at least for now that's the only place we'd use Registry.apply, just inlining it there:

return Registry.apiMethodRegistry[method].apply(null, allArgs).catch(...

Done in 705aed6 🚀

alexcjohnson · 2018-03-01T23:45:27Z

tasks/test_syntax.js

+        if(circularDeps.length) {
            console.log(circularDeps.join('\n'));
-            logs.push('some new circular dependencies were added to src/');
+            logs.push('some circular dependencies were found to src/');


found in src/

Good 👁️ done in d27b3ba

alexcjohnson · 2018-03-01T23:53:29Z

@etpinard this is brilliant. Beautiful. Heroic even. Amazing that even with the extra machinery it's a net decrease in LOC.

Just a few comments, the only substantive one being the Registry.call signature.

Love it. End of an era.

- move all spec under top-level describe(), so that they can easily reuse the 'unregister' wrapper, - fixup spec naming

... everywhere except for plots/command.js where and fn.apply would work better, for now just grab api method for registry cache.

... and 'enforce' Registry in components/modebar/ (like for all components really).

etpinard · 2018-03-02T18:09:16Z

A couple more things I want to add:

first, I'll reference https://github.com/plotly/plotly-webpack/issues/2#issuecomment-333256054 where the first observations on browser-pack-flat +/- circular dependencies were made,
second, errorbars and grid components are now totally optional and could be taken out of the core bundle in v2.
lastly, now that we're registering plot api methods, we could make some of them optional in v2 (i.e. move them out of src/core.js). If we choose to do, we'll just have to make sure the components that call api methods register them beforehand (similar to how trace modules register subplot modules).

alexcjohnson · 2018-03-02T19:17:12Z

src/plot_api/to_image.js


    return new Promise(function(resolve, reject) {
-        Registry.call('plot', [clonedGd, data, layoutImage, configImage])
+        plotApi.plot(clonedGd, data, layoutImage, configImage)


just curious, why did you choose not to use Registry here?

Plotly.toImage shouldn't be in a circular dep with the other Plotly methods. Plotly.toImage should call Plotly.plot but never the other way around.

Ideally, I think the api method registry should only be used in components/ and in the subplot modules (for interaction updates). Plotly methods need to call component methods (e.g .draw) and some components need to call Plotly methods (e.g. updatemenus / mode bar button click) - this is circular, hence the need for an api method registry.

alexcjohnson · 2018-03-02T19:24:30Z

💃 💃 💃

etpinard added 12 commits March 1, 2018 15:31

move grid module to components/

bee37dd

... so that its attributes are added to the schema free of circular deps.

merge src/plot_api/register.js into src/registry.js

973abbc

- no need to have a separate method just for the top-level method.

fixes #621 - make x/y/z dependent error bar attributes

b08c402

... by using utilizing component-to-schema logic, instead of punching the error bar attributes into the trace declarations.

use errorbars methods via getComponentMethod

87275af

... so that down the road errorbars could be taken out or src/core.js, and be its own registrable module.

add apiMethodRegistry in src/registry

52b83b3

- export api methods in plot_api/index.js - expose and register them in src/core.js

add Registry.call method + use it to 🔪 circular deps !!!

9fc8832

move Plotly.setPlotConfig to plot_api.js

381b6e4

use Registry.traceIs instead of plots.traceIs copy

ed8d56d

🔪 src/plotly.js !!!

204799c

🔒 down circular dep test at 0️⃣

2c529ec

update tests (mostly update ref to spyOn modules)

c6df699

add flaky tag to scattergl visible toggle test

011443e

etpinard added type: maintenance labels Mar 1, 2018

etpinard added this to the Decircularise src dependencies milestone Mar 1, 2018

alexcjohnson reviewed Mar 1, 2018

View reviewed changes

etpinard added 5 commits March 2, 2018 12:16

grab Grid module method using Registry.getComponentMethod

9246fda

fix typo in test_syntax

d27b3ba

rename register_test -> registry_test & improve it!

e05f4f0

- move all spec under top-level describe(), so that they can easily reuse the 'unregister' wrapper, - fixup spec naming

replace Registry.call('', []) with Registry.call('', ...[])

705aed6

... everywhere except for plots/command.js where and fn.apply would work better, for now just grab api method for registry cache.

no need for Registry in plot_api/to_image

a851aa5

... and 'enforce' Registry in components/modebar/ (like for all components really).

alexcjohnson reviewed Mar 2, 2018

View reviewed changes

etpinard merged commit 13eb982 into master Mar 2, 2018

etpinard deleted the zero-circular-dep branch March 2, 2018 19:34

etpinard mentioned this pull request Mar 6, 2018

Bundle with browser-pack-flat plugin #2447

Merged

Uh oh!

ZERO circular dependency #2429

ZERO circular dependency #2429

Uh oh!

Conversation

etpinard commented Mar 1, 2018

Uh oh!

nicolaskruchten commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Mar 1, 2018

Uh oh!

etpinard commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants