Material-ui-pickers: Using undocumented `externalDependencies` instead of `peerDependencies` potentially causing issues

[lostpebble]

Environment

| Tech | Version |
|---------------------|---------|
| material-ui-pickers | 1.0.0-rc.5 |
| material-ui | 1.0.0-beta.40 |
| React | 16.3.2 |
| Browser | Chrome |
| Platform | Windows 10 |

Steps to reproduce

1. Use yarn workspaces and install the required peer dependency of date-fns@^2.0.0-alpha.7 inside my /workspace/vs-frontend package.
2. Also rely on other packages which require date-fns, but an earlier incompatible version (webpack-cli for example).
3. Because yarn uses hoisting, it is hoisting material-ui-pickers up to a root node_modules folder, and the other date-fns packages, but leaving our specific version of date-fns@^2.0.0-alpha.7 behind because there is no known link between material-ui-pickers and that specific package and version.

Expected behavior

Busy getting feedback about the issue specifically relating to material-ui-pickers here: https://github.com/yarnpkg/yarn/issues/5705

Basically the expected behaviour would be that these two modules remain aligned in the same node_modules level.

Actual behavior

Instead I now have this folder structure:

  |- [email protected]
  |- vs-worker
  |    +- node_modules
  |        +- [email protected]
  |- vs-frontend
  |    +- node_modules
  |        +- [email protected]
  |- concurrently
  |- material-ui-pickers
  +- listr-verbose-renderer

Which is problematic for material-ui-pickers resolving the correct date-fns version. And I get this error:

Error: Cannot find module 'date-fns/addDays'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:548:15)
    ...

I'm busy getting feedback, but it seems that perhaps setting this external dependency correctly as a peerDependency might fix it. externalDependencies is not documented anywhere in the NPM spec, so I'm unsure why it is being used here.

[dmtrKovalenko]

Actually no. Because of for peer dependencies there is no ability to select one of date-fns, moment or luxon. Thus anytime date-fns users will have an error of not installed moment and luxon - thats would be an obvious and might be a reason of overheading installation.
externalDependencies uses just for our build system to exclude from bundle and mark libraries as external when we creating new build.

About different versions of date-fns - I cant imagine fast solution, because if libs are relying on different versions of the same library - how the peerDependencies will fix your problem

[lostpebble]

About different versions of date-fns - I cant imagine fast solution, because if libs are relying on different versions of the same library - how the peerDependencies will fix your problem

That's what I'm currently following up on here: https://github.com/yarnpkg/yarn/issues/5705

The general idea is that peerDependencies make an "official" link between material-ui-pickers and date-fns@^2.0.0-alpha.7. This would help package mangers know to place material-ui-pickers in a node_modules context in which it can access the correct version of date-fns.

The libs that require the different versions will generally then have their required version of date-fns as a sub-module, as is usual, such as: node_modules/web-cli/node_modules/date-fns because yarn or npm sees the clash. Currently because you are using an undocumented externalDependencies, we can't expect yarn or npm to ever recognise that relationship and see the problem.

Actually no. Because of for peer dependencies there is no ability to select one of date-fns, moment or luxon. Thus anytime date-fns users will have an error of not installed moment and luxon - thats would be an obvious and might be a reason of overheading installation.

Yea, I understand why this is an issue. This has been an issue for many when it comes to npm and peerDependencies - as seeing those warning messages for packages that you don't need installed is annoying.

But until npm has given us a better option, I think putting up with the warnings might be the best option. PeerDependencies are not installed automatically as far as I know, so it shouldn't cause overhead - only annoying warning messages.

[Haroenv]

What about publishing three versions of the library to npm, with the correct peerDependency (or actual dependency)?

[dmtrKovalenko]

@Haroenv thats should be a lot of libs to manage) I think would be more efficient to let npm write error messages.

[cherniavskii]

@Haroenv it's harder to maintain three versions :/

What if we remove externalDependencies?
We still have info about peer dependencies in Getting Started docs, and there's message in console after library is installed. It should be enough.

Also, what benefits has externalDependencies in package.json? If it just causes errors with yarn, it should be safe to remove it ;)

[dmtrKovalenko]

@cherniavskii externalDependencies its a custom section its not worked with yarn anyway. Yarn needs peerDependencies to resolve same library versions (date-fns v1 & v2) - and if some another lib relying on date-fns 1 its not working with material-ui-pickers.

[dmtrKovalenko]

@lostpebble what if you will say that you need date-fns 2 actually npm i date-fns@next --save

[cherniavskii]

externalDependencies its a custom section its not worked with yarn anyway

yeah, that's why I suggest to remove it ;)

[dmtrKovalenko]

@cherniavskii thats just a place to display that lib using some another library, because we are just ignoring moment, date-fns and luxon somewho will maybe looking for information about these libs in the package.json of installed package in node_modules.

[lostpebble]

@dmtrKovalenko that's what I've done, I've officially included it in my package.json as a direct dependency. But because of the way yarn hoisting works in workspaces, and because I have other modules which have the [email protected] library requirement (in their own package.json), it not seeing material-ui-pickers as requiring this specific version of date-fns@next, so it doesn't "hoist" them together and keep them in the same /node_modules context.

You can see in great detail what I'm talking about, as we've been discussing this exact problem with the yarn team over here: https://github.com/yarnpkg/yarn/issues/5705#issuecomment-383388063

In a regular project it wouldn't cause any issues, but in monorepos it is causing issues because of the way their algorithms work to preserve space. They would only ensure material-ui-pickers and date-fns@next will stay together if there is an officially defined link between them (peerDependencies).

So yea @cherniavskii , its not so much the issue of externalDependencies being there - but rather that peerDependencies is not being used instead.

[Haroenv]

I can imagine a build process which simply edits the package.json in a new folder for each of the versions and publishes all three with the same code but different package.json#peerDependencies to three packages. That would be a lot simpler. In a later stage I guess you could put the code that handles with the different dependencies in separate files too, which would lower the overhead of it (if that's necessary / makes sense)

[dmtrKovalenko]

I am actually aware of 3 packages - due to overhead. Using this lib should be as easy as possible. I will think about peerDependencies and maybe will include all libs to peer in the next release (rc.8)

I tend to the fact that we will not do that. Only one workaround I can imagine - create your own utils and use them. But a lot of errors for each build - not really good solution.

[RedHatter]

Another option would be to use the standard optionalDependencies field.

[cherniavskii]

@RedHatter thanks for sharing this info here, didn't know that optionalDependencies is standardized field.

@dmtrKovalenko I think we should use that, instead of externalDependencies :)

[dmtrKovalenko]

Moved to optional dependencies