Skip to content

Conversation

TrCaM
Copy link
Collaborator

@TrCaM TrCaM commented May 28, 2019

Description

Extend project scoped operations in the NodeJS Admin SDK with the addition of AppMetadata interface and 2 new methods into ProjectManagement class (unimplemented):

  • listAppMetadata() : Lists summary of all apps belongs to the project
  • setDisplayName(): Update display name of the project

In this pull request

  • Declare AppMetadata interface in AppMetadata.ts
  • Move AndroidAppMetadata and IosAppMetadata declaration into AppMetadata.ts
  • Make AndroidAppMetadata and IosAppMetadata inherited from AppMetadata
  • Update existing code and tests to match the change
  • Update types in index.d.ts (readonly will be added in a separate pr)
  • Declare listAppMetadata() and setDisplayName() which currently just throw errors.

@TrCaM
Copy link
Collaborator Author

TrCaM commented May 28, 2019

@hiranya911, I have a question for AppMetadata - Should we simply add the platform field to AndroidAppMetadata with the field's type requiring it to be set to ANDROID? Or should we convert AndroidAppMetadata to a class, and give it a constructor like below?

interface AppMetadata {
  public readonly appId: string;
  public readonly displayName?: string;
  public readonly platform: AppPlatform;
}

class AndroidAppMetadata implements AppMetadata {
  constructor(
      appId: string,
      displayName: string,
      readonly resourceName: string,
      readonly projectId: string,
      readonly packageName: string) {
    super(appId, displayName, AppPlatform.ANDROID);
  }
}

And would either of those options be considered a breaking change in the Admin SDK? There should be no reason for users of the SDK to ever construct an instance of AndroidAppMetadata themselves, but our public interface makes it possible to do so.

@hiranya911
Copy link
Contributor

Is there an API proposal for these changes?

I don't think what you've proposed above will actually work. Because AppMetadata is an interface, you cannot initialize its properties in AndroidAppMetadata by calling super(). You probably need to make AppMetadata a class for that to work.

@nbegley
Copy link
Contributor

nbegley commented May 28, 2019

@hiranya911, I'd like to re-pose @TrCaM's question after replacing interface AppMetadata with class AppMetadata. I'll send you a link to the private API proposal.

Basically, the question is "Can we change AndroidAppMetadata and IosAppMetadata from interfaces to "classes that inherit from AppMetadata" without that being a breaking change?

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just a couple of nits.

As for declaring the platform field, I like how it's currently done in the PR. It doesn't require us to make any drastic changes (some of which could be potentially breaking for some users), and gives enough type safety for the problem at hand.

@hiranya911 hiranya911 assigned TrCaM and unassigned hiranya911 May 28, 2019
@TrCaM TrCaM requested a review from hiranya911 May 29, 2019 13:08
@TrCaM TrCaM assigned hiranya911 and unassigned TrCaM May 29, 2019
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM.

Please make sure @nbegley signs off on this before merging.

@hiranya911 hiranya911 removed their assignment May 29, 2019
Copy link
Contributor

@nbegley nbegley left a comment

Choose a reason for hiding this comment

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

LGTM, with a small requested change.

@TrCaM TrCaM requested review from hiranya911 and nbegley May 30, 2019 19:00
@TrCaM TrCaM assigned hiranya911 and unassigned nbegley May 30, 2019
@TrCaM TrCaM force-pushed the tc-projectScopedOperations-start branch from f7ba7af to b9bf631 Compare May 30, 2019 19:28
@TrCaM TrCaM assigned nbegley and unassigned hiranya911 May 30, 2019
Copy link
Contributor

@nbegley nbegley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Tri!

@TrCaM TrCaM merged commit 1ff2e66 into tc-projectScopedOperations May 30, 2019
@TrCaM TrCaM deleted the tc-projectScopedOperations-start branch May 31, 2019 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants