Enhancement suggestions
lucas404x opened this issue · 4 comments
Hi there, I'm writing to you guys today to suggest some improvements to the SDK based on my experience using it.
- The Configurations class reads from appsettings.json, but not from appsettings.Development.json. It's essential this option because in appsettings we usually put production values while in appsettings.Development, well, the development values. As you guys have a sandbox option, I think that supporting both values makes sense.
- To create a
VonageClient
we need to pass aCredentials
object to the constructor, which makes sense. But as we already pass the ApiKey and the ApiSecrets to theappSettings
, why not add an option where theVonageClient
constructs theCredentials
object based on theConfiguration
class? It would be a really cool feature because today I need to read fromappSettings
to get these values and constructs theCredentials
object manually. - The last suggestion is to rename the "appSettings" to "VonageSettings" or something less generic. In projects with dozens of configs, these more specific names help a lot in navigating the project.
I hope that these suggestions are helpful. Thank you for your time and consideration.
Hi @lucas404x,
Thanks for all the feedback.
I'll try to provide answers below:
- That's a fair point. At the moment, we only from
appsettings.json
andsettings.json
(see here). I could do that, but given the file followsappsettings.{Environment}.json
, I cannot guarantee we follow the same format because everyone can name their environment according to their preference. Currently, I'm not sure I could use theEnvironment
variable as this is an external library. I'll have to test that. - Indeed, that's a good idea. I could provide a factory method that reads all the data from the configuration file. I'll fit that into my backlog. Regarding the client initialization, I just wanted to let you know that I added something recently where you can register the VonageClient in your service collection. It does allow you to inject the whole client or any sub client. You can have a look here
- That's a good point; I will keep that in mind. The tricky part is it's a breaking change for everyone, so I'll have to do that over the next major release (with a migration guide). I still need an ETA as it's not planned anytime soon. The reason is there are a lot of changes I'm willing to make, and I want to avoid making a few major releases in a row.
I hope I was able to answer your questions. I'll keep this issue open for tracking if it's alright for you.
Hi @lucas404x,
I'm just getting back to you about your suggestions. I was able to cook something for 1 & 2.
From now on, lazy initialization is available.
I exposed an override to the registration extension method. You can provide your configuration, meaning the settings file for your environment.
// For 'Scoped' lifecycle
builder.Services.AddVonageClientScoped(builder.Configuration);
// Foor 'Transient' lifecycle
builder.Services.AddVonageClientTransient(builder.Configuration);
All clients will be initialized with Credentials
built from your configuration values. You no longer have to handle it.
It allows you to have environment-specific appsettings.json
.
More details here.
This is not released yet, I assume I'll publish it this week.
Closing this issue, as points 1 & 2 were addressed.
I will adapt the codebase regarding point 3 in the next major release.