microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
964 stars 309 forks source link

[BUG] Typing incomplete on Templates (TypeScript, React) #1689

Closed ghost closed 1 year ago

ghost commented 2 years ago

Describe the bug Even though the documentation example utilizes a typescript project, it does not seem that this library is typescript compatible. Through a few test cases, the typing on objects is incomplete or incorrect.

To Reproduce Steps to reproduce the behavior:

Example 1:

Say you want to utilize a selected person that someone chooses through people picker

<PeoplePicker selectionChanged={(selection) => console.log(selection.detail)} />

Typescript will complain that TS2339: Property 'detail' does not exist on type 'Event'..

Example 2:

Try to use a template as described in the documentation (which is in a "tsx" format there)

import { MgtTemplateProps } from '@microsoft/mgt-react';

const MyEvent = (props: MgtTemplateProps) => {
  const { event } = props.dataContext;
  return <div>
    {event.subject}<br />
    {event.attendees
      .map((attendee: any) => attendee.emailAddress.name)
      .join(', ')}
  </div>;
};

You will receive errors such as "ESLint: Unsafe assignment of an any value. (@typescript-eslint/no-unsafe-assignment)"

Expected behavior If examples utilize Typescript then you would expect that the library actually supports Typescript.

ghost commented 2 years ago

Hello luffespresso, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

sebastienlevert commented 2 years ago

We support TypeScript (and we are in fact building in TypeScript). Which version of TypeScript are you targeting?

ghost commented 2 years ago

We support TypeScript (and we are in fact building in TypeScript). Which version of TypeScript are you targeting?

I am using 4.4.4 locally. Can you verify the examples I linked above do in fact work? I can troubleshoot on my end if it is a local issue, just want to ensure that the support is in fact there properly.

sebastienlevert commented 2 years ago

We'll validate the sample within the repo. They might be a bit outdated.

Can you test if this works for you : https://github.com/sebastienlevert/mgt-sandbox/tree/main/10-mgt-react

ghost commented 2 years ago

We'll validate the sample within the repo. They might be a bit outdated.

Can you test if this works for you : https://github.com/sebastienlevert/mgt-sandbox/tree/main/10-mgt-react

Your example "works" as in it runs, but even looking at this line: https://github.com/sebastienlevert/mgt-sandbox/blob/main/10-mgt-react/src/App.tsx#L59

the dataContext property has "any" type, so below that when you use sub properties of email, any TS linter would present you with errors such as "ESLint: Unsafe assignment of an any value".

The typing of the dataContext props is not present (at least not in the examples) so how does the user/IDE know what to expect when accessing properties of email in that case, such as email.sender.emailAddress.address?

gavinbarron commented 2 years ago

This is an interesting issue.

We generate the typing of events and properties on the React components based on the results that we get from the web components analyzer

I have an open issue on that repo asking for information on why we are not getting the types of the event to match the CustomEvent type that we are actually using and documenting. https://github.com/runem/web-component-analyzer/issues/248

gavinbarron commented 1 year ago

While looking at another matter I found @custom-elements-manifest/analyzer this might be what we need to close this issue

gavinbarron commented 1 year ago

@luffespresso I'm marking this closed this work has been done and is available as part of our v3.0.0-preview.1 release.