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

Client: Reduce Simple Dependencies #3450

Open
holgerd77 opened this issue Jun 11, 2024 · 11 comments
Open

Client: Reduce Simple Dependencies #3450

holgerd77 opened this issue Jun 11, 2024 · 11 comments

Comments

@holgerd77
Copy link
Member

Our client is up for some more serious production use cases with the rise of a stateless Ethereum world on the horizon and the need for lighter (browser) client (modules) coming along!

This is a warm-up task regarding bringing client dependencies to a bearable level, which is a necessary pre-requisite if we want the client being deployed and used in mission-critical environments!

There are likely various dependencies which can be easily removed and either replaced by some own code or "internalized" as we did in the past for other small libraries where the license permits it (e.g. MIT), e.g. crc32 in Common, see crt.ts.

On a rough first look these dependencies might be candidates (but definitely needs a second look/judgement, very quickly writing this together):

  • bodyparser
  • connect
  • cors
  • it-pipe
  • jwt-simple
  • qheap

(note that - for sure - yargs and winston will be candidates too to be handled, but these are more challening (might be solutions like the ability to replace the logger or something), and should not be part of this "warm-up session" 🙂 )

(another note: some dependencies (like level will likely/eventually also "fall away" coming with some client refactoring/modularization (so where e.g. a stateless client might not need a full level DB. But also something for later and more strucutred thoughts preparing for this 🙂 )

@holgerd77
Copy link
Member Author

Ah, also to note: please rather to be tackled "one by one" (so: one dependency, one PR, rare exceptions on this)

@foufrix
Copy link
Contributor

foufrix commented Jun 11, 2024

Hey, @holgerd77, I was taking a quick look at this. Most of the time, it depends on external packages as well. How do you want to proceed: start internalizing those and add new external or everything in one PR? But I suppose some have a lot of nested and sub-nested packages.

Example :

@holgerd77
Copy link
Member Author

Hey @foufrix, cool that you were having a look! 😄 Just to clarify: this was just a very quick write-up, I didn't have a closer look if removing the specifically cited dependencies is really an option respectively how we use these in the client code.

See the dependencies references rather as some "examples", this would still need a closer look one-by-one before doing anything (and it might very well turn out that the first look was too superficial and removing a certain dependency is not realistic).

Also note that this one is also really no short-term issue but a rather somewhat hard and strategic one, which will strain over months. We are generally cautious with adding dependencies, and most dependencies selected/used here will be here for some solid reason. So ease of removal will largely differ.

So generally we should go from simple to hard here.

Might be that you already picked some of the harder ones. Let me have a look.

Yeah, so connect seems already be a (too) hard one for now. I guess we should leave this aside for now.

This is deeply used within the code, not the "super core" part though but the RPC. This current issue comes along with an eventual option to further modularize the client, so that people can use selected parts (and with this skip certain dependencies). Also as some context. For RPC it might also be that we can offer this separately, and so also somewhat mitigate the need for people to draw too much dependencies in. Bigger topic though and something for some separate thought process.

For body-parser I have a lot more the impression that this is a viable direct candidate for replacement, "internalization" (copy the use code parts if possible by license an add a license header) or removal. This is only use in one very single place (and only for JSON parsing??) in src/util/rpc.ts and has an insanely large dependency list.

@foufrix
Copy link
Contributor

foufrix commented Jun 12, 2024

Hey @holgerd77, thanks for the quick reply,

Seems like cors is a good first candidate.
It depends on:

  • object-assign, which is only needed before node 4, and from what I see, we use node >18 so I can refactor and use directly Object.assign()
  • vary which has no dependencies.

Should I add vary.ts and cors.ts in packages/client/src/util?

Cors is only used in rpc.ts, so it ticks all the checkboxes for a first move.

Can you validate what I said, then I'll start the integration 🙏

@holgerd77
Copy link
Member Author

@foufrix Hi there, cool, yeah I would fully agree 🤩, great selection! Code is also small enough to not overload on our side, and it's also good to have this very old object-assign dependency out!

One "change request" is the comment I already gave here #3451 (comment) to Amir for the qheap internalization, so it would be good if you can place in a dedicated ext folder (guess it's ok to create in parallel, we'll for sure get this merged afterwards).

So I would think you can start here! 👍 If you want to be on the super-safe side you can wait for an additional comment from @acolytec3, he normally answers pretty quickly (will likely "come in" latest 2-4 hours after this comment) and I guess he has more insight in the code parts and sometimes he comes with some comments like "Oh, this (the cors) package is not so useful anyhow and I already wanted to remove" 😂 or something like that!

Guess chances here are only at 20% at most that this happens. So this should not hold you back if you would otherwise loose time you would want to spend on this!

@acolytec3
Copy link
Contributor

Did someone call? 🥷

So I have a thought about the whole set of deps we use in the rpc for middleware handling. Could we replace the whole set (connect, body-parser, cors) with something like ittyRouter, which has zero additional deps, is maintained (and modern and is written in Typescript), and looks like it has cors/json parsing middleware built in? I know that's a bigger project than just copy/paste cors to an internal folder but feels like it would be a bigger win in terms of shrinking our dependency footprint.

I'm not a master of API routing and how that would integrate with the jayson RPC module we use but it seems to be similar.

@g11tech any thoughts here?

@holgerd77
Copy link
Member Author

Did someone call? 🥷

So I have a thought about the whole set of deps we use in the rpc for middleware handling. Could we replace the whole set (connect, body-parser, cors) with something like ittyRouter, which has zero additional deps, is maintained (and modern and is written in Typescript), and looks like it has cors/json parsing middleware built in? I know that's a bigger project than just copy/paste cors to an internal folder but feels like it would be a bigger win in terms of shrinking our dependency footprint.

I'm not a master of API routing and how that would integrate with the jayson RPC module we use but it seems to be similar.

@g11tech any thoughts here?

Yes

@foufrix
Copy link
Contributor

foufrix commented Jun 14, 2024

I can take a look at ittyRouter implementation, effectively it solve 3 packages in one go. and maybe start with only the file rpc.ts where I can change connect, body-parser, and cors. As connect is used heavily everywhere else we can do it in two time, first in rpc.ts then the rest?

Until confirmation, I've started to move jwt-simple that has no sub depencies and is only used rpc.ts

@acolytec3
Copy link
Contributor

I think that's fine. I don't know for sure that ittyRouter is the solution here. Just did some googling for modern dependency free middleware providers and it came up and looks like it has the features we need.

I'll be honest that I'd much rather have one maintained, modern dependency (with no deps of its own) that takes care of all this super low level HTTP network layer stuff so we don't have to maintain it). Having to maintain two middleware modules to parse HTTP response bodies feels like work that we don't want to have to do (should the specs for them ever change).

@foufrix
Copy link
Contributor

foufrix commented Jun 23, 2024

To keep you updated, I've started to investigate how to add itty-router #3467, though I will not be able to continue before 9 July. If anyone wants to take a look before then, feel free! I will continue when I'm back.

@holgerd77
Copy link
Member Author

Cool! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants