-
Notifications
You must be signed in to change notification settings - Fork 3
New API to load from CDN endpoint #106
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
Conversation
src/load.ts
Outdated
| * @param endpoint The URL to the CDN. | ||
| * @param options Optional parameters. | ||
| */ | ||
| export async function loadCdn(endpoint: URL | string, options?: AzureAppConfigurationOptions): Promise<AzureAppConfiguration>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it makes more sense to name it loadFromCDN insead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use camelCase, so it will be loadFromCdn
25ae8a4 to
234ab73
Compare
src/load.ts
Outdated
| export async function loadFromCdn(endpoint: URL | string, options?: AzureAppConfigurationOptions): Promise<AzureAppConfiguration>; | ||
|
|
||
| export async function loadFromCdn( | ||
| endpoint: string | URL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it will be too much to call it cdnEndpoint or it's clearer? The reason I'm asking is because we also have the other parameter appConfigOptions. Will that confuse users that it's appconfig endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with cdnEndpoint. Updated.
src/load.ts
Outdated
| * @param cdnEndpoint The URL to the CDN. | ||
| * @param appConfigOptions Optional parameters. | ||
| */ | ||
| export async function loadFromCdn(endpoint: URL | string, options?: AzureAppConfigurationOptions): Promise<AzureAppConfiguration>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoint
Should this be updated too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird, I remembered I updated this in the commit, will update it again
788f3e0 to
cec0605
Compare
|
Verified Azure Frond Door + sas token can get kvs from app config. So merge this PR. |
This reverts commit 6dae81a.
No description provided.