darkroomengineering / lenis

How smooth scroll should be
https://lenis.darkroom.engineering
MIT License
7.33k stars 316 forks source link

Mark `react` and `react-dom` peer dependencies as optional. #329

Closed stefanobartoletti closed 2 months ago

stefanobartoletti commented 2 months ago

I noticed that after upgrading to the latest versions from 1.0.42, installing the base lenis also installs as new dependencies also those marked as peer, namely react and react-dom, that seem to be relevant only for the react-lenis package. Also, some other packages that seems only related to a React environment are pulled by default (i.e. Hamo and possibly others)

When using this library in other environments (i.e. I am using it with Vue and Nuxt), these dependencies should not be installed by default, and the optimal behavior should be previous one, as in 1.0.42 (only "plain" Lenis was installed).

If it matters, I'm having this behavior with pnpm

clementroche commented 2 months ago

Thank you for your feedback, what's the problem with having react, react-dom and hamo as dependencies since they will be tree-shaked anyway ?

I added them as dependencies to be sure that lenis/react can be used without any extra config required. Perhaps I could set react and react-dom as peerDependencies since it's obvious that you need them in order to use lenis/react.

stefanobartoletti commented 2 months ago

The fact is that even if these dependencies are not used, they still have to be resolved and downloaded. This is not really optimal, both on local development and especially when deploying, you will end using more build minutes and bandwidth than what the application really requires

This also may force other project dependencies to resolve these if they have it as optional deps too, in example this is what happens to another package that I have in my project before and after upgrading Lenis:

  '@tresjs/cientos':
    specifier: ^3.8.0
    version: 3.8.0(@tresjs/core@3.7.0)(three@0.163.0)(tweakpane@4.0.3)(vue@3.4.21)
  '@tresjs/cientos':
    specifier: ^3.8.0
    version: 3.8.0(@tresjs/core@3.7.0)(react@18.2.0)(three@0.163.0)(tweakpane@4.0.3)(vue@3.4.21)

Of course it is up to you and how you feel it is the best way to manage the project, but a sensible way would be to not forcibly include what is not actually used or needed. Lenis in itself is a great tool and really works wonderfully, but another of its selling-points IMHO is that does wonders while also being a very lightweight package (not only in the final bundle but in the whole process) and that it had exactly 0 dependencies.

If you set them as peer dependencies, you can consider setting them as optional, by using the required properties in package.json.

https://docs.npmjs.com/cli/v9/configuring-npm/package-json#peerdependenciesmeta

clementroche commented 2 months ago

I see @stefanobartoletti, I'll remove react and react-dom as dependencies and add them as peerDependencies, also I'll replace hamo with tempus which is lighter as dependency.

stefanobartoletti commented 2 months ago

Thanks for understanding and for your support.

I am available to further test this again after you update, if you want.

Have a nice day :v:

clementroche commented 2 months ago

@stefanobartoletti Can you help me figuring out why react is imported as dependency ? I've just checked and it's not a dependency of neither Lenis nor Hamo ?

Can you try v1.0.45-dev.0 ?

stefanobartoletti commented 2 months ago

I think that it depends on how each package manager works, I was testing with pnpm, and from what I can tell, setting them as peerDependecies should also be coupled with explicitly labeling them as optional.

I tried installing v1.0.45-dev.0 and looking at pnpm-lock.yaml I can see this

  /lenis@1.0.45-dev.0(react@18.3.0):
    resolution: {integrity: sha512-nbRqUgI05tuOfYLBQ2SaolYxgapHz66YB27RT3ngQapusR6lStTOw1AIkJHps7iCcYHbkaWXtM/gr/lowzIbLA==}
    peerDependencies:
      react: '>=17.0.0'
      react-dom: '>=17.0.0'
    peerDependenciesMeta:
      react:
        optional: true
      react-dom:
        optional: true
    dependencies:
      '@darkroom.engineering/tempus': 0.0.46
      clsx: 2.1.1
      react: 18.3.0
      zustand: 4.5.2(react@18.3.0)
    transitivePeerDependencies:
      - '@types/react'
      - immer
    dev: false

Looks like these deps are correctly marked as optional, but react is still brought in by zustand apparently.

clementroche commented 2 months ago

but zustand also marked react as optional https://github.com/pmndrs/zustand/blob/main/package.json#L267, i think it might be because of use-external store which is a dependency of zustand https://cdn.jsdelivr.net/npm/use-sync-external-store/package.json.

I think i can get rid off zustand actually

clementroche commented 2 months ago

1.0.45-dev.1 should fix it @stefanobartoletti, zustand has been removed

stefanobartoletti commented 2 months ago

Yes, 1.0.45-dev.1 is not bringing in react anymore.

Thanks for your support!

clementroche commented 2 months ago

https://github.com/darkroomengineering/lenis/releases/tag/v1.0.45