client: make prom client optional

[gabrocheleau]

This PR addresses a task that was mentioned in #3460, namely making the prometheus dependency optional.

[holgerd77]

Ah, very cool! 🤩

For now I would rather like to not make the depedency optional (yet) for comfortability reasons but keep in the normal set of dependencies, can we do that?

This is rather a conceptual thing within the code base so that we are prepared later on to easily remove (If/when we split packages out), and the code changes do count for this here.

Side question out of interest: I found/find this super interesting, that it is possible to type with promClient but only have the dependency in the optionalDependencies set, since this is at least touching one of the core conceptual problems I always have with this that the type would be nice to have but the dependency should be out.

Is this - to your knowlege - the minimal setup to achieve this? 🤔 Or is there any way to get the dependency out completely but keep the typing??

Or are there other ways to approach this? There is no way (in package.json) to "only reference the types" of a dependency, or something like that, right? Or would it be a way to "auto-generate" types for these cases from the original library (to get this done quickly) and include these??

Still one of the biggest structurally unsolved problems for me. 🤓

[acolytec3]

Ah, very cool! 🤩

For now I would rather like to not make the depedency optional (yet) for comfortability reasons but keep in the normal set of dependencies, can we do that?

This is rather a conceptual thing within the code base so that we are prepared later on to easily remove (If/when we split packages out), and the code changes do count for this here.

Side question out of interest: I found/find this super interesting, that it is possible to type with promClient but only have the dependency in the optionalDependencies set, since this is at least touching one of the core conceptual problems I always have with this that the type would be nice to have but the dependency should be out.

Is this - to your knowlege - the minimal setup to achieve this? 🤔 Or is there any way to get the dependency out completely but keep the typing??

Or are there other ways to approach this? There is no way (in package.json) to "only reference the types" of a dependency, or something like that, right? Or would it be a way to "auto-generate" types for these cases from the original library (to get this done quickly) and include these??

Still one of the biggest structurally unsolved problems for me. 🤓

I don't remember exactly how it all worked (and don't have the energy to do the git archaeology this early in the morning) but this is basically what we did with our DB back when we had the specific level dep instead of the abstracted class we have now. We just had an interface defined that imported the level types and then just expected an object passed in that conformed to those types. I think maybe the difference there is that level did/(does?) has a specific types package that is separate from the main level.js dep that we used. Not sure if something similar exists for prometheus or not.

[gabrocheleau]

Ah, very cool! 🤩

For now I would rather like to not make the depedency optional (yet) for comfortability reasons but keep in the normal set of dependencies, can we do that?

This is rather a conceptual thing within the code base so that we are prepared later on to easily remove (If/when we split packages out), and the code changes do count for this here.

Side question out of interest: I found/find this super interesting, that it is possible to type with promClient but only have the dependency in the optionalDependencies set, since this is at least touching one of the core conceptual problems I always have with this that the type would be nice to have but the dependency should be out.

Is this - to your knowlege - the minimal setup to achieve this? 🤔 Or is there any way to get the dependency out completely but keep the typing??

Or are there other ways to approach this? There is no way (in package.json) to "only reference the types" of a dependency, or something like that, right? Or would it be a way to "auto-generate" types for these cases from the original library (to get this done quickly) and include these??

Still one of the biggest structurally unsolved problems for me. 🤓

We certainly can leave that as a dependency, and eventually decide to make it optional. It does make the conditionality checks irrelevant (since it will always be present), but at least that part of the work will be done. Wouldn't it make sense for this to be done "all at once" though rather than in 2 steps?

Concerning the optionalDependencies and their related types, I've researched this a bit today and it seems like the proper approach would be:

• Make the actual dep an optional dependency
• Install the types as a devDependency

Otherwise we would have (from what I'm reading) to do some conditional magic for the types as well in case they have not been installed.

[holgerd77]

Hmm, but how would this work for TypeScript users of the library with types as a dev dependency? 🤔 Wouldn’t this break their code?

[gabrocheleau]

Hmm, but how would this work for TypeScript users of the library with types as a dev dependency? 🤔 Wouldn’t this break their code?

Not sure I understand what you mean. Aren't types always just used as dev dependency since they're only needed during development and not in the production runtime environment?

[holgerd77]

Hmm, but how would this work for TypeScript users of the library with types as a dev dependency? 🤔 Wouldn’t this break their code?

Not sure I understand what you mean. Aren't types always just used as dev dependency since they're only needed during development and not in the production runtime environment?

No, at least as far as my understanding goes (correct me if I have some thought error along the way), types are shipped as these d.ts files for people who want to work with a dependency within their own TypeScript dev environments. And these type files reference/import the respective types from the respective libraries we are talking about.

So here is e.g. a similar example from Blockchain (since I am working on that right now), where the _ethash property is typed with Ethash:

If we would now directly remove the dependency (or make it optional) we will break people's TypeScript code from the default installation, right?

That's why I have e.g. replaced this with a minimal interface here:
https://github.com/ethereumjs/ethereumjs-monorepo/pull/3504/files#diff-0d1fc3cbd7363c27c9d15a33f9271bbf4fa0d4b4671f7f0b4be5faca3241031aR7

But that's also where I am hanging a bit. So this is simple to be done for dependencies with a very low/small surface API but otherwise things get hairy quickly.

Or is the way to go here to include the definitely-typed type package (so here e.g.: for prom) to the production dependencies?

[gabrocheleau]

That's why I have e.g. replaced this with a minimal interface here:

I think the example you mention is a different scenario, where we were actually instantiating the class here: new Ethash(this.blockchain!.db as any), hence it was necessary to include the full dependency. Now this has been replaced with a constructor arg, which is typed with the minimal interface. A middle-ground more similar to what I'm suggesting iwth prom could have been to:

• Include the types only as a devDependency
• Add the constructor arg with the EthHash type, removing the need to instantiate the actual class

For something like prometheus, I think the API surface is more substantial, so creating a minimal interface might not be feasible. I'll look into it again.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (ef20930) to head (357227b).
Report is 10 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (ef20930) and HEAD (357227b). Click for more details.

HEAD has 5 uploads less than BASE

Flag	BASE (ef20930)	HEAD (357227b)
util	1	0
devp2p	1	0
common	1	0
statemanager	1	0
trie	1	0

Additional details and impacted files

Flag	Coverage Δ
common	?
devp2p	?
statemanager	?
trie	?
util	?
wallet	0.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.