microsoft / nova-facade

Runtime support for the. JS Nova pattern, which allows sharing of High-Value-Components across M365
MIT License
20 stars 25 forks source link

Feature request: Enable nova data façade to support partial implementations of the interface #31

Open ionmorozan opened 2 years ago

ionmorozan commented 2 years ago

Hi,

Scenario Org Explorer is an Apollo graphql people experience (aka.ms/orgexplorer) where my team integrated People Highlights (ref on scenario here). Our integration package is implementing the nova data interface.

Background info Today highlights are integrated in the host app (OrgX) root query. OrgX is a NON-nova host app. Highlights type is embedded inside OrgX root query making the implementation of useLazyLoadQuery difficult. so far we opted for implementing the following methods, while querying happens inside the host app and then it's passed down as props:

Challenge Since data is passed down via props, we can't ensure proper usage of useFragment unless we extract from the Fragment$key the data and forcefully cast it to a ref.

const highlightRef = props.personaHighlightFragment[" $data"] as unknown as {
    readonly " $fragmentRefs": FragmentRefs<"HighlightsFragment">;
  };

where personaHighlightFragment is a Nova fragment constructed this way (on the host app)


 // Convert from Apollo fragment to Nova(Relay) fragment
  const personaHighlightFragment: NovaHighlightsFragment$key = {
    " $fragmentRefs": { HighlightsFragment: true },
    " $data": personaHighlight as NovaHighlightsFragment,
  };

Ask: Enable nova to handle such object transformations when data is not fetched via useLazyLoadQuery but rather passed via props.

kerrynf commented 2 years ago

@alloy @ionmorozan Was this discussed in more detail? Did we resolve this request?

ionmorozan commented 2 years ago

@kerrynf , short answer is no. We have not taken this to the next level of details. Today we opted for casting the hostApp specific fragment to a Nova defined fragment. See below what I actually mean by it.

   // Compose the highlight fragment ref.
  // This is a workaround to mimic the response we would get from useLazyLoadQuery.
  // https://github.com/microsoft/nova-facade/issues/31
  const highlightRef = props.personaHighlightFragment as unknown as {
    readonly " $fragmentRefs": FragmentRefs<"HighlightsFragment">;
  };

  return (
    <HighlightsPopupHintProvider {...props}>
      <Highlights highlightRef={highlightRef} size={props.size} />
    </HighlightsPopupHintProvider>
  );
sjwilczynski commented 1 year ago

@kerrynf, @alloy +1 on this issue. This is particularly a problem when you want to create some simple stories in Storybook for a component that gets data through useFragment. Let's take a component as simple as:

export type PersonProps = {
  person: Person_personFragment$key;
};

export const personFragment = graphql`
  fragment Person_personFragment on Person {
    defaultName {
      displayName
    }
  }
`;

export const Person = (props: PersonProps) => {
  const person = useFragment(personFragment, props.person);
  const name = person.defaultName?.displayName ?? "";
  return (
    <div>
      <Avatar
        className={styles.avatar}
        name={name}
        active={"active"}
        activeAppearance={"shadow"}
        color={"neutral"}
        shape={"circular"}
        size={40}
      />
        <Text>{name}</Text>
    </div>
  );
};

Normally I would like to be able to pass data through args and it is possible as useFragment by default would pass whatever is put in as fragmentRef. But to get typescript to be happy we need to add cast as unknown as Person_personFragment$key, which makes stories not typesafe because with explicit cast like this we won't get any type check failures on changing fragments (removing/adding fields)

export default {
  component: Person,
  decorators: [/* some decorators*/],
};

type Story = ComponentStoryObj<typeof Person>;

export const Primary: Story = {
  args: {
    person: {
      defaultName: {
        displayName: "John Doe",
      },
    } as unknown as Person_personFragment$key,
  },
};

Maybe we could have some utility function that would allow us to create data for fragment declaratively but still be able to satisfy type system?

sjwilczynski commented 2 days ago

Storybook part is solved with the decorator available in @nova/react-test-utils. As for partial implementation of the interface we haven't discussed this further but I'd say we shouldn't allow to pass fragment raw prop, it should come from some graphql query executed somewhere