Skip to content

fix: fix types to match current api response#26

Merged
michaeljolley merged 5 commits into
mainfrom
lo/issue-25/changing-types-to-match-api-output
Mar 1, 2022
Merged

fix: fix types to match current api response#26
michaeljolley merged 5 commits into
mainfrom
lo/issue-25/changing-types-to-match-api-output

Conversation

@lukeocodes

Copy link
Copy Markdown
Member

updated the API key type to match the current API response

fixes #25

@lukeocodes lukeocodes added the bug Something isn't working label Feb 25, 2022
@lukeocodes lukeocodes requested review from a team and michaeljolley February 25, 2022 20:55
briancbarrow
briancbarrow previously approved these changes Feb 25, 2022
@lukeocodes

Copy link
Copy Markdown
Member Author

@michaeljolley the question is if this should be a major version bump or whether we should parse the response and output the original format for compatibility.

What do you think?

@michaeljolley michaeljolley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments, then realized this is the wrong way to fix this issue. In this PR, we're modifying the Key type. That type is currently used by the KeyResponse type and the Keys static class as the return type of the create & get functions. Modifying the Key type would provide the incorrect return type for those methods.

Instead, we should create a new Member type and add it as an optional property to the KeyResponse type, like:

import { Key } from "./key";
import { Member } from "./member";

/**
 * Response from the Deepgram API to list keys
 */
export type KeyResponse = {
  /**
   * Array of API keys associated with the project
   */
  api_keys: Array<{
    /**
    * Optional member associated with the API key
    */
    member?: Member;
    
    /**
    * API key
    */ 
    api_key: Key;
  }>;
};

Comment thread src/types/key.ts Outdated
Comment thread src/types/key.ts Outdated
Comment thread src/types/key.ts Outdated
Comment thread src/types/key.ts Outdated
};
/**
* API key to send in API requests (Only displayed when first created)
* Api key object

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API (capitalized since it's an abbreviation)

@michaeljolley

Copy link
Copy Markdown
Contributor

@lukeocodes answering the other question around SemVer: ideally we would retain the old typing and provide the previous output as well (if possible) and flag the properties we know are wrong/outdated as deprecated to be removed at a future date.

@lukeocodes

lukeocodes commented Feb 26, 2022

Copy link
Copy Markdown
Member Author

Ahh, yeh I see it now.

@lukeocodes

Copy link
Copy Markdown
Member Author

@lukeocodes answering the other question around SemVer: ideally we would retain the old typing and provide the previous output as well (if possible) and flag the properties we know are wrong/outdated as deprecated to be removed at a future date.

I'm not sure I know how to achieve this without introducing a promise to wrap around _request in order to modify the output, with some property to switch to the new output?

@lukeocodes

lukeocodes commented Feb 26, 2022

Copy link
Copy Markdown
Member Author

Types for Key and Member now match the API response.

  • Changed the existing Member type as it didn't appear to be used anywhere else yet
  • Changed the response type and body of list to maintain the existing output, while the typing of the API response is now accurate to the API reference and the body coming back.
  • Added a deprecation notice. We can kill this commit separately if this is the wrong way to deprecate the output for a major release later.

@michaeljolley michaeljolley merged commit eec0cde into main Mar 1, 2022
@michaeljolley michaeljolley deleted the lo/issue-25/changing-types-to-match-api-output branch March 1, 2022 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Output of keys list doesn't match docs

3 participants