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

Reorganize internal dependencies #4478

Closed
8 of 9 tasks
simonbrunel opened this issue Jul 9, 2017 · 6 comments
Closed
8 of 9 tasks

Reorganize internal dependencies #4478

simonbrunel opened this issue Jul 9, 2017 · 6 comments
Assignees

Comments

@simonbrunel
Copy link
Member

simonbrunel commented Jul 9, 2017

The actual code base doesn't import features but fully relies on Chart.* (e.g. Chart.helpers, Chart.defaults, etc...). Files are imported in chart.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 of Chart.{feature} and so remove all function export and Chart.* references.

var helpers = require('../helpers');    // instead of = Chart.helpers
var defaults = require('../defaults');  // instead of = Chart.defaults
//...

The Chart namespace would be fully populated in chart.js:

// chart.js
var Chart = require('./core/core.controller');
Chart.defaults = require('./core/core.defaults');
Chart.helpers = require('./helpers/index');
Chart.controllers = require('./controllers/index');
// ...

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:

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?

@salzhrani
Copy link

This is super awesome.
I would also suggest switching to rollup once all is done.

@simonbrunel
Copy link
Member Author

It's planned :)

@pndewit
Copy link

pndewit commented Oct 4, 2017

Any progress on this? I'd love to see this one added to the 2.8 milestones! 👍

@etimberg
Copy link
Member

etimberg commented Oct 4, 2017

@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.

@yahya-uddin
Copy link

Any progress on this?

@benmccann
Copy link
Contributor

The code uses import everywhere now, so I'm going to consider this done. #6935

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

No branches or pull requests

6 participants