Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

De-circularize plotly.js require statements #236

Closed
etpinard opened this issue Feb 3, 2016 · 3 comments · Fixed by #2429
Closed

De-circularize plotly.js require statements #236

etpinard opened this issue Feb 3, 2016 · 3 comments · Fixed by #2429

Comments

@etpinard
Copy link
Contributor

etpinard commented Feb 3, 2016

@mdtusz

Latest list of circular dependencies:

  • Plots < --- > Annotations
  • Plots < --- > Legend
  • Plots < --- > Axes (the only Plots calls in axes.js are traceIs)
  • Plots < --- > plotApi
  • plotApi < --- > Annotations
  • plotApi < --- > Shapes
  • plotApi < --- > Titles
  • plotApi < --- > Fx
  • plotApit < --- > Legend

Possible partial solutions:

  • Make a component registry and register the components (e.g. Annotations, Shapes, Titles, Fx, Legend), so plot_api.js doesn't have to require the full component modules (similar to trace and subplot modules). Blocking: we'll need a centralized api for the component module, e.g. all component modules should have a draw method and possible and init method.
  • Use a more OO pattern (in the long term). Attach relayout and restyle to a plot object so that files requiring plot_api.js (e.g Annotations, Shapes and Fx) can use gd.relayout() instead of plotApi.relayout(gd, ...).
@etpinard
Copy link
Contributor Author

The World Calendar push of #1220 added a new circular dependency pattern to src/:

image

as src/registry.js requires Lib for and src/lib/dates.js requires Registry to use the calendars component methods.

To solve this, we can either:

  • make lib/dates.js a component module (as one could argue that src/lib/ files should have to use the registry)
  • require lib functions directly in registry.js instead of requiring the whole Lib module

etpinard added a commit that referenced this issue Feb 17, 2017
@alexcjohnson
Copy link
Collaborator

In the course of #1403 I made some circular deps involving Lib. In the short term I'm fixing these by breaking up Lib and just requiring the pieces I need, but longer term perhaps the right solution is to only allow things in Lib that do not require code from elsewhere in plotly.js. That would make it safe to import Lib anywhere, without forcing us to break it into a million little files. Would be a bit of an effort though...

@etpinard
Copy link
Contributor Author

Refer to PR #2032 for the most up-to-date findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants