Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a new "request-network-sdk" package that bundles all the packages for interacting with the protocol. #1181

Closed
MantisClone opened this issue Sep 29, 2023 · 6 comments
Assignees

Comments

@MantisClone
Copy link
Member

MantisClone commented Sep 29, 2023

Problem(s)

  1. It's unclear what the term "Request Network SDK" refers to. It's only an internal term that we use to describe the Request Network packages.
  2. Installing the Request Network SDK requires many separate package installations.

Possible Solution

Create a new "request-network-sdk" package that bundles all of the packages that Builders typically install so that they only need to install 1 package to get all of the Request Network functionality in their app. The new package should be on the requestNetwork monorepo, not in a new repository.

What if they don't want ALL the functionality in their app?

We can tree-shake so the unused packages are removed from the final app.
We can't yet tree-shake so the unused modules within packages are removed from the final app. #1148 will add this ability to tree-shake at the module level.

@MantisClone MantisClone transferred this issue from RequestNetwork/docs.request.network Sep 29, 2023
@MantisClone MantisClone changed the title Create a new "Request Network SDK" package that bundles all the packages for interacting with the protocol. Create a new "request-network-sdk" package that bundles all the packages for interacting with the protocol. Sep 29, 2023
@skiv71
Copy link
Contributor

skiv71 commented Oct 12, 2023

Discovered package dependancy issues for @requestnetwork/payment-processor, when including it as a dependancy of the new SDK. Several of it's own dependancies (currency, smart-contracts, utils etc.) are not resolving, causing tsc build to fail. Requires investigation (ongoing).

@benjlevesque
Copy link
Contributor

Discovered package dependancy issues for @requestnetwork/payment-processor, when including it as a dependancy of the new SDK. Several of it's own dependancies (currency, smart-contracts, utils etc.) are not resolving, causing tsc build to fail. Requires investigation (ongoing).

@skiv71 I made a comment on your PR that could explain your issues

@benjlevesque
Copy link
Contributor

About point 2/, there might be an easy first step: @requestnetwork/epk-decryption, requestnetwork/epk-signature and @requestnetwork/web3-signature could be dropped and merged into @requestnetwork/request-client.js, as they don't add any extra dependency

Regarding the solution of a "SDK" package, I believe the name isn't appropriate. The requestNetwork SDK is a set of packages, what you want is an "umbrella" package, or starter for dapps. Some proposals: @requestnetwork/dapp-sdk, @requestnetwork/dapp-client, @requestnetwork/starter.

Having all dependencies installed "by default" would make installation slow (nb: it already is, but this is being worked on) and might cause dependency conflicts (version of ethers, of bn.js...). I definitely agree that we can improve the DX to get started with the SDK, but it's rather common to have more than 1 package to install to get started. Actually lots of libraries require this (for instance, this comes from the NestJS docs: $ npm i --save @nestjs/core @nestjs/common rxjs reflect-metadata

@MantisClone
Copy link
Member Author

About point 2/, there might be an easy first step: @requestnetwork/epk-decryption, requestnetwork/epk-signature and @requestnetwork/web3-signature could be dropped and merged into @requestnetwork/request-client.js, as they don't add any extra dependency

I think we'll create new decryption mechanisms, starting with @requestnetwork/lit-decryption and then later @requestnetwork/eip-5630 in RequestNetwork/encrypt-requests-with-wallet-prototype#3. We might create a new signature mechanism @requestnetwork/safe-multisig-signature in #1174 For this reason, I don't have any issue with the signature and decryption mechanisms staying in separate packages.

Regarding the solution of a "SDK" package, I believe the name isn't appropriate. The requestNetwork SDK is a set of packages, what you want is an "umbrella" package, or starter for dapps. Some proposals: @requestnetwork/dapp-sdk, @requestnetwork/dapp-client, @requestnetwork/starter.

I suppose. The "Request Network SDK" could be "all the packages in the monorepo" and the "Request Network dApp SDK"
could be an umbrella package of the basic packages needed to build a dapp. @skiv71 What do you think?

Having all dependencies installed "by default" would make installation slow (nb: it already is, but this is being worked on) and might cause dependency conflicts (version of ethers, of bn.js...). I definitely agree that we can improve the DX to get started with the SDK, but it's rather common to have more than 1 package to install to get started. Actually lots of libraries require this (for instance, this comes from the NestJS docs: $ npm i --save @nestjs/core @nestjs/common rxjs reflect-metadata

@skiv71 was passionate that adding an umbrella package would be helpful for DX and convinced me that the effort was small and worth implementing sooner rather than later. If your argument is that it would have been better to wait on this task, I might agree, but at this point, we might as well continue. If you are against this change for a different reason, then please let me know.

@MantisClone
Copy link
Member Author

This task motivated a lot of good discussion between @skiv71 and me. We concluded that creating an umbrella package is not worth doing at the moment, despite the time spent so far.

@MantisClone
Copy link
Member Author

MantisClone commented Oct 19, 2023

I previously closed it as completed, but it should have been closed as not planned.

@MantisClone MantisClone closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants