date-fns is a hard dependency of this adapter

PowerKiKi commented 3 years ago

This has the benefit of both correctly installing required packages for end-user, as a well as formalize which version of date-fns is supported or not.

simonbrunel commented 3 years ago

I don't think it should be a dependency but instead a peer dependency, as it's done for the luxon and moment adapters.

PowerKiKi commented 3 years ago

I disagree, npm official documentation explain that peerDependencies is used when your package is a plugin for something else. And chartjs-adapter-date-fns is not a plugin for date-fns. It absolutely requires date-fns or else it would crash and have absolutely no purpose at all. So date-fns must be installed, and the only way to enforce that, before npm 7, is via dependencies (because not everybody is on npm 7 yet, or npm at all).

And because date-fns follows semantic versioning, there is no risk of duplicates of date-fns in node_modules, as long as we use a version range wide enough. And that's the case with "^2.0.0". We allow all possible versions of compatible date-fns since two years ago.

There really is no reason to force our users to manually add one extra dependencies to their project when we can automate it for them (for npm < 7, yarn, pnpm, and others).

(the dependency management in node ecosystem is a mess, and the trend to use peerDependency everywhere is not helping)

simonbrunel commented 3 years ago

It absolutely requires date-fns or else it would crash and have absolutely no purpose at all

This also applies to the chart.js peer dependency. It's an adapter between these two libraries and I would consider both the same, especially since they are both external.

...there is no risk of duplicates of date-fns in node_modules, as long as we use a version range wide enough

I just made a quick experiment with concurrently, which has "date-fns": "^2.16.1" in dependencies. I got mixed results depending on how I was declaring the "date-fns": "2.18.0" version in my package.json, but also if it was locked or not in package-lock.json. With npm 14, many times it fails to dedup the date-fns dependency, meaning concurrently was using the latest version (2.22.0) while my project was importing 2.18.0. I remember we got similar complains in the past.

Using peer dependencies indeed requires our users to explicitly install date-fns but I don't think that's a big deal compared to have them figuring out dependency duplication. Many adapters adopt the same strategy and I would recommend the use of peer dependency in this case.

@kurkle thoughts?

PowerKiKi commented 3 years ago

I'd be interested in a reproducible case where you saw duplicated modules. Did you try running npm dedupe that was made specifically to sort out those kind of situation ?

But even though there might be cases where duplicates happens, I don't think it would be a problem for the specific case of date-fns, wouldn't it ? what could break if both versions are used at the same time ?

kurkle commented 3 years ago

I agree this needs to be changed. I incorrectly moved it to devDependencies when I added a bundled build: https://github.com/chartjs/chartjs-adapter-date-fns/commit/06c79b6b510676b14b49ba8b0f0be79270f26998

Its correct for the bundle, but incorrect for the rest. (The bundle could be removed as date-fns is again available in CDN)

Some thoughts to the dependency vs peerDependency debate (great explanation in SO).

There have been no complains in moment or luxon adapters from the peerDepency
If the consuming project uses date-fns for something else too, it would seem the peerDependency is the correct palce.
If date-fns is only used by the adapter, peerDependencies will work the same, except you'll still have to install date-fns manually (except with npm 7 that does this for you)
I'd tend to think more people are familiar with npm i than npm dedupe

The locale support example is an example case where duplication could be a problem (the locale coming from different version than the adapter is using).

To the version number, we do not know if this adapter will work with date-fns v3. We could assume it will, and only fix it if it doesn't. So maybe just >=2.0.0 could be most verstatile, allowing people test it with pre-releases when they are out etc.

simonbrunel commented 3 years ago

I'd be interested in a reproducible case where you saw duplicated modules.

I'm wondering if in the case where date-fns is a (strong) dependency of the adapter (as "date-fns": "^2.0.0"), what happens if a user wants to use a pre-release of date-fns? Are you sure that the following setup will not generate duplicates, but more importantly, will the adapter actually use the pre-release since ^2.0.0 doesn't include pre-releases?

{
  "dependencies": {
    "chartjs-adapter-date-fns": "^2.0.0",
    "date-fns": "2.23.0-beta.1",
  }
}

...what could break if both versions are used at the same time ?

I'm not familiar with date-fns (so I don't know if it applies here) but one issue could be that classes will not be the same between different versions (e.g. a instanceof A will return false if a is constructed from class A of version 2.18.0 while A is imported from version 2.18.1 for the check. That becomes an issue if your project uses a lib that exposes date-fns instances that you will inject in your chart.js data.

chartjs / chartjs-adapter-date-fns

date-fns is a hard dependency of this adapter #39