search

XRPLF / xrpl.js

A JavaScript/TypeScript API for interacting with the XRP Ledger in Node.js and the browser
https://xrpl.org/
1.19k stars 507 forks source link

Move `ws` package out of `@xrplf/isomorphic` #2660

Closed mahnunchik closed 4 months ago

mahnunchik commented 4 months ago

Hello,

Thank you for v5 release! It is big step!

I've faced that @xrplf/isomorphic depends on network related package ws (huge).

I'm using only crypto related things:

  "dependencies": {
    "ripple-address-codec": "^5.0.0",
    "ripple-binary-codec": "^2.0.0",
    "ripple-keypairs": "^2.0.0"
  },

I think there is sense to move ws package to separate folder/module. Thus, only network-bound modules will depend on it.

ckniffen commented 4 months ago

It will not be a runtime dependency unless you direcly import it due to how tree-shaking is setup.

mahnunchik commented 4 months ago

Yes, it is true. But I think there is sense to separate crypto primitives (really small module in v5) and network helper to separate packages.

For the dependency list example above, I will never use the network but ws part of @xrplf/isomorphic module is always installed.

justinr1234 commented 4 months ago

@mahnunchik I appreciate your suggestion and concern, but for the purposes of the maintainability of the project, the ws package is very small and as @ckniffen mentioned with tree shaking bundle sizes in a non-node environment are not impacted.

For some background context, there was previously debate and discussion about separating packages out in the first place. It was settled that this approach, for now, was the right balance between optimization and maintainability.

That said, if there is enough community interest for this change, feel free to have more people show up and comment and it can be reconsidered. For now, I'll be closing this issue out for maintainability of repo hygiene. Thanks again for making the suggestion!

justinr1234 commented 4 months ago

@mahnunchik Additionally, you may be able to follow this guide on forcibly removing nested packages - https://stackoverflow.com/questions/36758157/how-can-i-forcibly-exclude-a-nested-dependency-for-npm