ZeeCoder / use-resize-observer

A React hook that allows you to use a ResizeObserver to measure an element's size.
MIT License
651 stars 42 forks source link

Do not save the library as devDependency #39

Closed ashubham closed 4 years ago

ashubham commented 4 years ago

This will be shipped in production for most use cases, the Docs indicates to save as a devDependency

ZeeCoder commented 4 years ago

It's assumed you use some form of bundling, like webpack, so it's the bundle you should ship, not the raw lib.

On Fri, 1 May 2020, 22:43 Ashish Shubham, notifications@github.com wrote:

This will be shipped in production for most use cases, the Docs indicates to save as a devDependency

You can view, comment on, or merge this pull request online at:

https://github.com/ZeeCoder/use-resize-observer/pull/39 Commit Summary

  • Do not save the library as devDependency

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ZeeCoder/use-resize-observer/pull/39, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4CKEVSRE3YQHJVIS4XRYLRPMYA5ANCNFSM4MXLJM7Q .

ashubham commented 4 years ago

Yeah, you would use some bundler I agree. But if you add it as a devDependency, then if someone does an npm i --production (on the application it will only install dependencies), the use-resize-observer will be omitted from the install.

So semantically its not correct to add production dependencies to devDependency. devDependencies are reserved for things which do not end up in the bundle like linter/compiler etc.

ZeeCoder commented 4 years ago

You seem to have a different workflow from what I've used in the past so far.

For me: prod deps = shipped deps for the node server, while Dev deps are stuff like linters and bundle related stuff, that's only ever used during development.

when you do the prod install, and skip those deps, that's exactly what I expect.

even if I have a php backend, and don't actually use any node packages in production, I'd still install them as dev dependencies, because that's literally what they are: dependencies that are not needed in production.

Regardless, I think people can decide for themselves how they want to install it, I'd they have a different workflow like you.

On Mon, 4 May 2020, 23:18 Ashish Shubham, notifications@github.com wrote:

Yeah, you would use some bundler I agree. But if you add it as a devDependency then if someone does an npm i --production on the application it only installs dependencies. So semantically its not correct to add production dependencies to devDependency. devDependencies are reserved for things which do not end up in the bundle like linter/compiler etc.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ZeeCoder/use-resize-observer/pull/39#issuecomment-623711114, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4CKEULWFD2FGY3IKG4SPDRP4WJBANCNFSM4MXLJM7Q .

ZeeCoder commented 4 years ago

The only exception I can think of is of you have an electron setup with a duo package format, where you'd ship everything within app/, and you configure your bundler to keep the import statements instead of actually bundling in the lib, in which case you do actually need to ship used libraries.

At which point it's a proper production dependency, as it needs to be present for stuff to work.

ashubham commented 4 years ago

Well that's exactly what I meant, 'devDependencies' are not shipped along with the product.

I think useResizeObserver is shipped along as well. Thus, useResizeObserver should be installed as a dependency and not as a devDependency. For the same reason you put React as a dependency and not as a devDependency.

ashubham commented 4 years ago

FWIW, Bundler is an artificial tooling construct. People are moving to bundlerless development, and there these semantics make more sense.

ZeeCoder commented 4 years ago

Well, thanks for the input. 👍 I'm sure whoever will use these "bundlerless" stuff will know what to do.

ashubham commented 4 years ago

Seems there are better libraries available to do what your package does in a more semantically correct way .. notice they use npm i react-use instead of saving it as a devDep. I will switch to that.

https://github.com/streamich/react-use/blob/master/docs/useMeasure.md

Thanks for the clarification! 👍

ZeeCoder commented 4 years ago

I honestly feel it's a bit comical to think even that how one describes installing the library to be an issue... Install it however you want mate. 😅

And feel free to switch if that's such a blocker to you. 👍

ashubham commented 4 years ago

Well it was never a blocker, I made an effort to fix the minor something I saw. Which I still feel is the right thing to do. I just lost confidence in the library maintainer to make our project depend on it.

ZeeCoder commented 4 years ago

All right then. 😅

On Mon, 18 May 2020 at 22:41, Ashish Shubham notifications@github.com wrote:

Well it was never a blocker, I made an effort to fix the minor something I saw. Which I still feel is the right thing to do. I just lost confidence in the library maintainer to make our project depend on it.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ZeeCoder/use-resize-observer/pull/39#issuecomment-630424114, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4CKEXN7TH2Q7BETVVETRTRSGMPJANCNFSM4MXLJM7Q .