Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

feat: abstract google & facebook account as external_accounts, fix (d… #14

Merged
merged 1 commit into from
Apr 2, 2021

Conversation

yourtallness
Copy link
Contributor

…e)serialization for public & private metadata

BREAKING: GoogleAccount resource is no more, replaced by ExternalAccount.

Also serializes public / private metadata to string to match the format the server expects.

Public / Private metadata converted to Record<string, unknown> (from object) to enable attribute access.

@SokratisVidros
Copy link
Contributor

@yourtallness We should do the same in the go SDK about google and facebook, right?

Copy link

@teoulas teoulas left a comment

Choose a reason for hiding this comment

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

Looks great 👍 - just a tiny issue with the canned JSON responses.

src/__tests__/apis/responses/getUser.json Outdated Show resolved Hide resolved
@yourtallness
Copy link
Contributor Author

@yourtallness We should do the same in the go SDK about google and facebook, right?

I checked the go sdk with @alex-ntousias and the external_accounts are not typed there, so currently nothing to do there.

@yourtallness yourtallness force-pushed the yourtallness/external_account_support branch 6 times, most recently from 87f0b03 to 499f8d9 Compare March 31, 2021 11:51
Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@alex-ntousias alex-ntousias left a comment

Choose a reason for hiding this comment

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

Nicely done @yourtallness . Happy for this to get merged. 💯 :shipit: 🚀

| primaryEmailAddressID | string |
| primaryPhoneNumberID | string |
| publicMetadata | Record<string, unknown> |
| privateMetadata | Record<string, unknown> |
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Out of curiosity, what is Record?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to SO:

Record is JavaScript's Object used as a map (key-value pair).

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -37,6 +36,16 @@ export class UserApi extends AbstractApi {
userId: string,
params: UserParams = {}
): Promise<User> {
// The Clerk server API requires metadata fields to be stringified

if (params.publicMetadata && !(typeof params.publicMetadata == 'string')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Why do you check if the public metadata is already string? To cover for cases where the user has already stringify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have at least one known case of a dev passing it stringified.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@clerk clerk deleted a comment from yourtallness Apr 1, 2021
…e)serialization for public & private metadata
@yourtallness yourtallness force-pushed the yourtallness/external_account_support branch from 499f8d9 to 0a9d071 Compare April 2, 2021 16:49
@yourtallness yourtallness merged commit 9117769 into main Apr 2, 2021
@yourtallness yourtallness deleted the yourtallness/external_account_support branch April 2, 2021 16:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants