search

flekschas / regl-scatterplot

Scalable WebGL-based scatter plot library build with Regl
https://flekschas.github.io/regl-scatterplot/
MIT License
185 stars 21 forks source link

nit: Duplicate peer-dependencies #128

Closed insertmike closed 1 year ago

insertmike commented 1 year ago

nit: New versions of NPM handle peer dependencies IIRC. In this case, they do not need to be installed explicitly:

In README.MD: 

_FYI, regl-scatterplot doesn't bundle regl and pub-sub-es, which is why you have to install them separately._

This could lead to some locked version of pub-es or regl that mismatch with regl-scatter.

flekschas commented 1 year ago

I not 100% sure I understand what the issue is. regl-scatterplot is treating pubsub-es and regl as peer dependencies because wrapper applications might use them as well. This might be less of an issue for pubsub-es but bundling regl twice is unnecessarily blowing up the final bundle size.

This could lead to some locked version of pub-es or regl that mismatch with regl-scatter.

What do you mean with "locked version"?

insertmike commented 1 year ago

@flekschas (see 2nd comment first)

As of npm version 7 (2021), peer dependencies are installed automatically from package.json. This means that they do not longer need to be added explicitly.

NPM Doc Ref

On the other side, that would be a pity to debug if someone tries to install and experience some issues because of his old NPM version. And we might need to note this at the README which makes the removing of the peer-dependency comment not so useful.

What I meant however about the 'locked version' is that we may encounter the following scenario:

Someone with old version of node installs pubsub-es explicitly. A new version of regl-scatter comes up which relies on a breaking change in pubsub-es. If the version of pubsub-es is locked in the package manager lock file (e.g yarn.lock) as the old one (not explicitly as latest, this would essentially break the project using the library.

To conclude I think this is really nit and we should not focus on this until it becomes problem, we can close it

insertmike commented 1 year ago

Actually, @flekschas , looking at the package.json I see that pub-sub-es and regl are both added as peer-dependencies as well as dependencies. Why is that? If this is the case, why would we need to explicitly tell people to install them?

flekschas commented 1 year ago

In order to locally test regl-scatterplot you need the two libraries. However during bundling they are excluded. See https://github.com/flekschas/regl-scatterplot/blob/master/rollup.config.mjs#L22-L25

flekschas commented 1 year ago

Closing this issue now for the reasons explained in #130 (https://github.com/flekschas/regl-scatterplot/pull/130#issuecomment-1604349710)

Happy to reopen it if I missed something.