-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Reorganize internal dependencies #4478
Labels
Comments
This is super awesome. |
It's planned :) |
This was referenced Jul 15, 2017
Any progress on this? I'd love to see this one added to the 2.8 milestones! 👍 |
@pndewit would be a nice feature, unfortunately both @simonbrunel and I are crazy busy right now so we aren't able to do it ourselves. |
This was referenced Jan 7, 2018
This was referenced Apr 1, 2018
This was referenced Nov 28, 2018
This was referenced Jan 1, 2019
Any progress on this? |
The code uses |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The actual code base doesn't import features but fully relies on
Chart.*
(e.g.Chart.helpers
,Chart.defaults
, etc...). Files are imported inchart.js
in the order they are supposed to depend from each other. As far as I tried, this makes very complicated to safely and properly import a subset of the library.We could reorganize the library to use
require('path/to/features')
instead ofChart.{feature}
and so remove all function export andChart.*
references.The
Chart
namespace would be fully populated inchart.js
:Removing the "function export" impacts a lot of lines (remove one tab almost everywhere), so we could do that step by step to avoid huge and painful reviews and limit conflicts with open PRs. I would sequentially split the work as follows:
MakeChart.helpers
importable #4479 exportChart.helpers.*
, importsrc/helpers/index
MakeChart.defaults/Ticks/Interaction
importable #4512 exportChart.defaults.*
, importsrc/core/core.defaults
Chart.plugins
importable #5114, MakeChart.Animation/animations/Tooltip
importable #5382, Make the main controller importable #5969 exportChart.*
from core classesMakeChart.Element/elements.*
importable #4540 exportChart.elements.*
, importsrc/elements/index
Make Chart.controllers.* importable #5871 exportChart.controllers.*
, importsrc/controllers/index
Cleanup scales export for better import strategy #5953 exportChart.scales.*
, importsrc/scales/index
MakeChart.platform
importable #4509 exportChart.platform.*
, importsrc/platform/platform
MakeChart.plugins
importable #5114 exportChart.plugins.*
, importsrc/plugins/index
Deprecate Chart.{Type} classes #5868 deprecateChart.*
(from `src/charts/*.js)This is a preliminary work in order to migrate to ES6 modules (discussed in #2466, #4461, #4303, etc.). Awesome work as been done by @salzhrani but I believe that trying to do everything in one step (including ES6 new features) will take time, be hard or even impossible to review / handle conflicts with incoming contributions and finally takes forever to land in master.
Thoughts?
The text was updated successfully, but these errors were encountered: