Skip to content
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

fix: add Group entity to IDynamicPerson type and introduce typeguards to find the entity type #2688

Merged
merged 19 commits into from Sep 28, 2023

Conversation

Mnickii
Copy link
Collaborator

@Mnickii Mnickii commented Aug 29, 2023

Closes #2649

PR Type

Bugfix

Description of the changes

Update the IDynamicPerson type with Group entity
Include entityType as a way to find the selected person type

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

@microsoft-github-policy-service
Copy link
Contributor

Thank you for creating a Pull Request @Mnickii.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@Mnickii
Copy link
Collaborator Author

Mnickii commented Aug 29, 2023

@sebastienlevert from your proposal you'd suggested using personType as the property added, but Person entity already has personType as a property , which can be of a class Person or Group.
I've renamed this to entityType that can be "user" | "people" | "group", would this work? Then it can be set where "personType in personDetails" as entityType: person and "groupTypes in personDetails" as entityType:group cc @gavinbarron

@sebastienlevert
Copy link
Contributor

Renaming means breaking change. @gavinbarron do you have a suggestion for this case? We need to identify a way to know if a picked entity is a group or a person.

@github-actions
Copy link

The updated storybook is available here

@Mnickii Mnickii marked this pull request as ready for review September 5, 2023 08:46
@Mnickii Mnickii requested a review from a team as a code owner September 5, 2023 08:46
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

The updated storybook is available here

@gavinbarron
Copy link
Member

gavinbarron commented Sep 5, 2023

Renaming means breaking change. @gavinbarron do you have a suggestion for this case? We need to identify a way to know if a picked entity is a group or a person.

Adding a new property, which is what I think @Mnickii has done is not a breaking change though.

Adding to the type guard idea you can factor out the typing even further:

type IUser = (MicrosoftGraph.User | MicrosoftGraph.Person) & { entityType: 'user' }
type IContact = MicrosoftGraph.Contact & { entityType: 'contact' }
type IGroup MicrosoftGraph.Group & { entityType: 'group' }

export type IDynamicPerson = (
  IUser
  | IContact
  | IGroup
) & {
  /**
   * personDetails.personImage is a toolkit injected property to pass image between components
   * an optimization to avoid fetching the image when unnecessary.
   *
   * @type {string}
   */
  personImage?: string;
};

isGroup = (obj: IDynamicPerson): obj is Group => {
    return entityType === 'group';
};

@gavinbarron
Copy link
Member

Actually, on reflection, perhaps the better approach here is to provide an exported isGroup type guard function for developers to use should they need to determine an object emitted in an event is a group or not? Thoughts?

@sebastienlevert
Copy link
Contributor

I like this last proposal @gavinbarron!

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

1 similar comment
@github-actions
Copy link

The updated storybook is available here

@sebastienlevert
Copy link
Contributor

To illustrate this scenario, could we build a simple story?

Co-authored-by: Gavin Barron <gavinbarron@microsoft.com>
@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

Great work on simplifying by removing the entityType property.
Now we need to clean up the casting that was introduced when we had the entityType property.

@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

1 similar comment
@github-actions
Copy link

The updated storybook is available here

@Mnickii Mnickii changed the title fix: add Group entity and entityType property to IDynamicPerson type fix: add Group entity to IDynamicPerson type and introduce typeguards to find the entity type Sep 28, 2023
@github-actions
Copy link

The updated storybook is available here

Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

Fantastic stuff, this give us a nice clean mechanism for understanding the type of a result with minimal code changes.

I really appreciate the work that you put in to get this one over the line.

@gavinbarron gavinbarron merged commit b3bc50d into main Sep 28, 2023
7 checks passed
@gavinbarron gavinbarron deleted the bug-2649 branch September 28, 2023 17:41
gavinbarron pushed a commit that referenced this pull request Sep 28, 2023
… to find the entity type (#2688)

add Group type to IDynamicPerson
implement type guards
refactor getInitials to use type guard functions
add entityType story
export typeguards
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
4 participants