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

Implement adapter to abstract date/time features #5960

Merged
merged 7 commits into from
Jan 11, 2019

Conversation

simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Jan 6, 2019

Edit: #6016 adds support for adapter options

In short, what are the main differences with #5522

  1. this PR doesn't enforce Luxon (or moment) to use our time scale
  2. thus, it doesn't encourage people to integrate more libs in the core build
  3. it doesn't add extra dependencies (even optional) to the core build
  4. it converts the time scale to use timestamps only (thanks @kurkle)
  5. it allows per adapter unit tests (Luxon, moment, etc.)

In details

  1. some users requested integrations with other libs than luxon (e.g. date-fns or dayjs). I don't think we should decide for them what's the best date/time lib they should use. Not everyone will need timezone or locales support and would prefer a faster/smaller lib than Luxon. Also, many users already picked a date lib for their projects and don't necessary want to depend on another one "considered" better.
  2. wrappers (or adapters) would increase the size of the core build, so better to have them implemented externally and avoid rejecting PR for integrating this or that date lib. It also allows faster iteration, independent releases and tickets.
  3. the "optional" nature of moment wasn't a feature but a misused configuration of Browserify allowing to skip moment in the browser but didn't allow UMD consumers to properly provide a custom version of moment when loaded inside a Node or require.js project. Switching to rollup made the moment dependency mandatory for CJS projects, we still need to fix it.
  4. the time scale doesn't anymore manipulate luxon or moment objects but timestamps, which also mean we don't expose these objects publicly and so will not have to maintain compatibility with luxon if we need to change lib.

How adapters work

  • external adapters should override methods in Chart._adapters._date (private).
  • the time scale calls chart._adapters._date methods to manipulate timestamp.
  • the adapter API is currently private to not have to maintain it until stabilized.

For example, the Luxon adapter (chartjs-adapter-luxon)

How it works

// in the browser
<script src="https://cdn.jsdelivr.net/npm/luxon@1.8.2/build/global/luxon.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/chart.js@2.8.0/dist/Chart.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/chartjs-adapter-luxon@1.0.0"></script>

Timezone, locales and extra time features

When using adapters, the user have full control over the date lib and so can configure it globally. If at some point we get requests to have these configurations per charts, we will introduce adapter specific options that will be passed to the adapter parse / format methods (e.g. scale.options.adapter.date.* or whatever name). It's not part of this PR because we may not need this granularity, so let's wait until it's explicitly requested. Anyway, the time scale shouldn't have to deal directly with such features, neither try to abstract them.

Closes #5522

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor notice on deprecated functionality.

src/scales/scale.time.js Outdated Show resolved Hide resolved
src/core/core.adapters.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

I believe we need to move moment to peerDependencies in package.json in order to make it optional for users of npm builds

@simonbrunel
Copy link
Member Author

I believe we need to move moment to peerDependencies

Correct, but since we need to generate Chart.bundle.*.js, we need it as a strong dependency. Maybe moving it in peer + dev dependencies would make it works as expected.

src/scales/scale.time.js Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member Author

I believe we need to move moment to peerDependencies ...

That could also break some integrations because they will now need to explicitly depend on moment. Our dependency is ^2.10.2 which should still allow npm users to specify a custom version for moment.

@benmccann
Copy link
Contributor

Yes, you're right, it would need to be added to both peerDependencies and devDependencies to correctly include it in the bundle. I pulled down this PR and confirmed it works when you do that, but not when it's in peerDependencies alone

It is true that users will need to explicitly depend on moment, but they will receive a warning like the following letting them know if they don't, so it's pretty low risk:

npm WARN chart.js@2.8.0 requires a peer of moment@>=2.10.2 but none is installed. You must install peer dependencies yourself.

If we don't make that change I fear it removes most of the benefit of this PR, which is that people have been asking to switch out moment for smaller libraries (#4303). Without updating the package.json we don't allow them to do that, but only allow them to include multiple date libraries, which would only increase the size even more

@simonbrunel
Copy link
Member Author

I pulled down this PR ...

Does it work as expected with the luxon adapter?

... it removes most of the benefit of this PR

You right, the ultimate goal is to allow CJS/ES users to skip moment from their builds but I think this is another fight, too complicated to be part of this PR. Note that users of the UMD build as global (<script>) will not need to include moment, but we still need to find a solution for CJS/ES users.

... they will receive a warning like the following letting them know if they don't, so it's pretty low risk

I just want to make sure we don't break CI of our users and unfortunately, that's just a warning: the build will success and will be deployed without moment, breaking their project, so we need to consider that use case.

If everyone agree with the approach of that PR, we could investigate the moment optional case in a separate PR because we also need to figure out how to make it optional in the UMD build consumed as CJS module.

etimberg
etimberg previously approved these changes Jan 9, 2019
src/chart.js Outdated Show resolved Hide resolved
kurkle
kurkle previously approved these changes Jan 10, 2019
nagix
nagix previously approved these changes Jan 10, 2019
Copy link
Contributor

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to say how happy I am about this PR. I had problems replacing moment with dayjs and this new adapter architecture would help a lot. Can't wait to see it merged 🥇

Also I'm very curious on how this would get adopted for the coming v3 release. I expect something like this:

import chartjsAdapterMoment from 'chartjs-adapter-moment';
var chart1 = new Chart(ctx, {
    dateTimeAdapter: chartjsAdapterMoment //default
});

src/core/core.adapters.js Show resolved Hide resolved
src/adapters/adapter.moment.js Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member Author

Also I'm very curious on how this would get adopted for the coming v3 release

In 2.8+, you will not be able to use moment for one chart and luxon for another chart in the same project. We think it's not a common use case, so we decided to not support per chart adapter since it would require more logic and a public API to maintain. Only one adapter should be imported, overriding the default one (which is currently moment for backward compatibility).

This work great when imported globally (<script>):

<script src="https://cdn.jsdelivr.net/npm/luxon@1.8.2/build/global/luxon.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/chart.js@2.8.0/dist/Chart.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/chartjs-adapter-luxon@1.0.0"></script>

And for CJS / ESM projects, it "should" work like:

import Chart from 'chart.js';
import 'chartjs-adapter-luxon';  // or 'chartjs-adapter-dayjs'

var chart1 = new Chart(ctx, {
   // ...
});

The adapter is imported for "side effects" (implicitly registered). Though, I'm not totally convinced it should be implicit, we may want to prefer the following approach, however it's a bit weird since the adapter is a singleton.

import Chart from 'chart.js';
import ChartAdapterLuxon from 'chartjs-adapter-luxon';  // or 'chartjs-adapter-dayjs'

// NOT THE CURRENT IMPLEMENTATION!
Chart.adapters.date = ChartAdapterLuxon;

var chart1 = new Chart(ctx, {
   // ...
});

@simonbrunel simonbrunel dismissed stale reviews from nagix, kurkle, and etimberg via b76d6b4 January 10, 2019 11:04
@mojoaxel
Copy link
Contributor

In 2.8+, you will not be able to use moment for one chart and luxon for another chart in the same project. We think it's not a common use case, so we decided to not support per chart adapter since it would require more logic and a public API to maintain. Only one adapter should be imported, overriding the default one (which is currently moment for backward compatibility).

👍

Thinking out loud:

  • It really makes no sense to load different adapters in the same project/page. Therefore an implicit loading is propably the easiest thing from a user perspective (in v3).
  • Probably the first adapter I'm going to need is an chartjs-adapter-empty to get rid of moment.js completely. Of course this only makes sense if the time-scale is not used, at all.
  • I really can't wait writing my first adapter 🚀

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @simonbrunel !

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 11, 2019
@simonbrunel simonbrunel merged commit 8a3eb85 into chartjs:master Jan 11, 2019
@simonbrunel simonbrunel deleted the chore/date-adapter branch January 11, 2019 07:03
@mojoaxel
Copy link
Contributor

🎉 Thanks! 🏅

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants