Azure / AppConfiguration-JavaScriptProvider

The configuration provider for consuming data in Azure App Configuration from JavaScript applications like Node or browser apps.
https://github.com/Azure/AppConfiguration
MIT License
7 stars 2 forks source link

Refactor AzureAppConfiguration with new interfaces #49

Closed Eskibear closed 6 months ago

Eskibear commented 8 months ago

Design

We support both two approaches to access key-values, maximize compatibility with customer's application code. For easier adoption of Azure App Configuration, and less code change.

AzureAppConfiguration {
  get<T>(key: string): T | undefined;  // preferred method, e.g. config.get("app.settings.color")
    constructConfigurationObject(options);    // helper function to construct hierarchical object, compatible with chained dot notation, e.g. config.app.settings.color
}

Edge case (for using configuration object only):

Hierarchical key-value pairs with overlapped key prefix:
  key: "app3.settings" => value: "placeholder"
  key: "app3.settings.fontColor" => value: "yellow"

How to re-construct data object from key-values?
config.app3.settings = ? 

In this case, constructConfigurationObject() throws an error.

Mitigation approach

Add an option skipObjectValidataion: boolean, default to false. - true. Throw an error when constructing the data object., because of ambiguity. - false. Skip the validation, allow information lost in data object, as the preferred get() method can always have the expected results. (propose to use constructConfigurationObject(options) as below, which would not affect the original load() call)

Eskibear commented 8 months ago

Offline synced with Zhenlan, two improvements

Updates from design review:

rossgrambo commented 8 months ago

AppConfig impl Map interfaces

I don't think there is a Map interface. Javascript added Map, and Javascript doesn't have interfaces. Looks like Typescript didn't add an interface for it as far as I can tell.

Nvm- zhiyuan found it

zhiyuanliang-ms commented 8 months ago

For the edge case:

Edge case (for data property only): Hierarchical key-value pairs with overlapped key prefix: key: "app3.settings" => value: "placeholder" key: "app3.settings.fontColor" => value: "yellow"

How to re-construct data object from key-values? config.data.app3.settings = ?

Since the default shape is map and user needs to call constructDataObject(options) to use data object, is this a valid case? If the user sets two key "app3.settings" and "app3.settings.fontColor", will he really use the data object instead of map to get configuration?

If the user wants to use data object, he should set the value as json shape and set the content type as json.

zhiyuanliang-ms commented 8 months ago

@rossgrambo There is Map interface in TypeScript.

My question is why to use a custom IGettable interface and throw exception when set is called. Currently, we are using ReadOnlyMap interface, this makes more sense to me.

Now I see the reason why we implement a custom IGettable as Get<T> will convert the value to the T type.

Eskibear commented 7 months ago

As the design is finalized, this PR is ready for review now. Please take a look.