Replace direct with peer dependency on moment.js

[ovidiubute]

This library depends on moment.js both as a direct dependency and as a peer dependency. Because react-dates has picked this library up a consuming application will end up with two moment.js packages in its bundle.

I don't see a reason why moment.js needs to be both a direct and a peer dependency in this library, but maybe I'm missing something, I'd love to clarify that.

If it's just an oversight I'll be glad to contribute a PR to remove moment.js from the direct dependency list and just leave it as a peer dependency.

[CalebMorris]

See commit message at react-moment-proptypes#7924af433.

It was removed at one point, but was leading to other problems due to how the peer dependencies function (see the above link for details there).

Ultimately there shouldn't be a problem with two packages in the bundle since the >= version limitation should allow consuming the same package so long as the externally included package isn't below 1.6.0.

If you have a specific version of NPM (or other package manager) that is pulling multiple versions of the package, would you mind describing the setup and version?

[ovidiubute]

My issue is not related to dependency management in the sense of installation or version of npm/yarn. It's related to how this library is bundled together with moment.js. Because of that, if you have a web-app, and you compile your bundle with webpack (for example), and you depend on both moment.js and react-dates, webpack will include both the moment.js package you installed, and the one brought by this library.

In my opinion a wrapper library such as this one that augments functionality of a certain library should not be bundled together with said library. That's why peer dependencies exist, to avoid the issue.

As for actually installing and working on this repo, what is common practice is to specify a devDependency (instead of a direct one) along with a peerDependency. When you work on the repo you install it, but when you ship it to npm the code requires consumers to provide their own version of moment.js.

This pattern is very common in libraries that depend on React, for example, because having two instances of React in a web-app is a real issue and the React maintainers actually check that you cannot load two Reacts in a single bundle. This has led to an enforcement of the pattern that I have just described.

[ovidiubute]

Picture's worth a thousand words (I cannot share the entire set-up as it's from my work). These images are captured from a webpack bundle analyzer. You can see that I have two instances of moment.js in my bundle.

moment.js transitively brought by this library:

moment.js I installed myself:

[CalebMorris]

A couple questions and points to clarify and see if I'm understanding the problem here. If any of this is incorrect please point it out so I can get a better grasp here.

Clarifications

Depedencies

I'm not bundling any dependency with this library. You would see the usage of bundledDependencies if that was the case.

---

In my opinion a wrapper library such as this one that augments functionality of a certain library should not be bundled together with said library. That's why peer dependencies exist, to avoid the issue.

This is not how peerDependencies are intended to be used and let to the original removal and re-adding to dependencies. They are a description of optional dependencies in the form of compatibility to allow the dependency manager to pull in a compatible version of the lib if it has a dependency. This library has a required dependency on moment and so it should live in dependencies.

The usage of >= allows for any consumers of the library to specify a specific version of moment and react-moment-proptypes will use that unless it is a really old version. react-dates has a transitive dependency on moment and doesn't really need to declare a direct dependency so chooses to go with peerDependencies so that anyone using it can be aware of the features they feel need to be present. So long as no downstream dependency requires a specific version the latest will be pulled in. This is the desired behavior.

The pattern you link in react-redux again is as a plugin and compatibility situation. It doesn't actually depend on react except for tests (devDependencies), but wants to provide a way of message that there are incompatible versions that shouldn't be consumed with this lib.

Bundling

[...] you compile your bundle with webpack (for example), and you depend on both moment.js and react-dates, webpack will include both the moment.js package you installed and the one brought by this library.

The bundling software will only include multiple versions of moment.js here is you have multiple versions installed. Webpack (and most bundlers) have tree-shaking that will prune any unused software.

Example package-lock.json when installing just moment and react-moment-proptypes:

{
  "name": "react-moment-proptypes_46",
  "version": "1.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "moment": {
      "version": "2.22.2",
      "resolved": "https://registry.npmjs.org/moment/-/moment-2.22.2.tgz",
      "integrity": "sha1-PCV/mDn8DpP/UxSWMiOeuQeD/2Y="
    },
    "react-moment-proptypes": {
      "version": "1.6.0",
      "resolved": "https://registry.npmjs.org/react-moment-proptypes/-/react-moment-proptypes-1.6.0.tgz",
      "integrity": "sha512-4h7EuhDMTzQqZ+02KUUO+AVA7PqhbD88yXB740nFpNDyDS/bj9jiPyn2rwr9sa8oDyaE1ByFN9+t5XPyPTmN6g==",
      "requires": {
        "moment": "2.22.2"
      }
    }
  }
}

When you bundle this with Webpack you only get the one moment.js copy included.

webpack bundle analyzer

I created a gist with a simple Webpack bundle and direct install of react-moment-proptypes and moment and only see 1 version of moment.js included.

Even the analyzer only shows 1 included.

From the images you include above I also only see 1 moment included.

If you have 2 versions installed you see something different:

Questions

• Are you explicitly bundling a moment.js copy through Webpack or referencing the one in your node_modules/?
  • If using the installed version, can you put the package-lock file here so I can see what's going on?
• Can you create a sample project that has multiple moment.js libs included so I can experiment?

[ovidiubute]

I'm directly depending on moment.js: 2.21.0. Since this library depends on ^2.22.0 webpack cannot "tree-shake" (the term is actually deduplication, tree shaking occurs only in true ES6 modules, I'm still delivering a CommonJS bundle), and so I have two moment.js dependencies in the final bundle: 2.21.0 which I installed (using an exact dependency), and a 2.22.0 or up (patch anchored) that react-dates depends transitively due to this library directly depending on moment.js ^2.22.0.

I can build a repo with a minimal reproduction if you think my explanation is not enough.

[ovidiubute]

I'm not bundling any dependency with this library. You would see the usage of bundledDependencies if that was the case.

It's a failure on my side to correctly describe the case: this library directly depends on moment.js, a bundler will structure the final bundle by including moment.js since it is a direct dependency.

I'm well aware of the fact that module bundlers can and do de-duplication of packages, but in my scenario, described above, webpack cannot do that since I depend on an older version. I believe this could have been avoided, but if you think otherwise, I'll probably just upgrade my version of moment to 2.22.x and be done with it.

[CalebMorris]

The use-case as you describe should only have one reference to moment.js.

If you have following dependencies

{
  "moment": "2.21.0",
  "react-moment-proptypes": "^1.6.0",
}

you will end up with a single reference to moment.

---

[...] react-dates depends transitively due to this library directly depending on moment.js ^2.22.0.

I'm not sure what you mean by this. This library depends on moment >= 1.6.0 which should allow for the reduction of included moment.js references to a single package (the directly referenced one).

I don't see how it's possible for react-dates could possibly include multiple moment packages because of my library. My library is the only one of their dependencies that directly pulls in moment, but the specifics of which package is allowed is very loose and should dedupe for moment@2.21.0.

I really do want to understand what's going on here.
If it isn't too much trouble would you please create an example package?

Correction

I wasn't aware that the term deduplication was used in terms of package managers. I've only ever heard tree-shaking used here. Thanks for the new info.

[ovidiubute]

No worries, I'll create a repo and try to reproduce. It is entirely possible my problem isn't caused by this library, hope I don't sound too angry over the Internet 😄 I'll ping you when it's up.

[CalebMorris]

I would like to bring this issue back to attention, though my use case is a bit different. I'm relying on a globally provided moment instance, which is bundled in a different package unrelated to mine.

My bundle also relies on external react and react-dom instances for example. This is working fine since all react libraries I use specify react as a peerDependency, and thus react is not bundled.

With your library though it is different and moment still gets installed and bundled. This really should not be the case and moment should only be specified as a peerDependency. I should be able to create a bundle without moment.

I am using webppacks externals feature to load those dependencies at runtime.

I am not sure I can follow your reasoning for specifying moment as a dependency.

--Jannik Keye (Mon, Aug 26, 2019 at 5:58 AM)

[CalebMorris]

In response to Jannik's comment:

You can read the above discussion about the purposes of dependencies and peer-dependencies. Forcing a change in how something is configured to appease a particular application configuration is not something I see as add-value and actually induces more work for a common user.

As for your specific problem, depending on the bundling tool you are using, you should be able to strip out dependencies as part of the bundle configuration.