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(fast-components): add missing type info to registration functions #5473

Closed

Conversation

EisenbergEffect
Copy link
Contributor

@EisenbergEffect EisenbergEffect commented Dec 16, 2021

Pull Request

📖 Description

This PR fixes an issue with our component registration functions where type information was being lost in certain cases. This caused downstream effects in our React wrappers, which could not be accurately typed, and were then unusable in React.

🎫 Issues

👩‍💻 Reviewer Notes

The changes are applied to the index.ts files of all the FAST Components. You will see that I have explicitly declared the type params for all calls to compose. There are two reasons for this:

  1. Some components have custom options, specified with the first type param. Due to a limitation in TypeScript, if there's a function with two type params, once the first param is specified, it can no longer infer the second type param. Because our component registries with custom options were not specifying the second param, the type information was not flowing through the registry. This resulted in an inability to produce type-accurate React wrappers. So, I've added both type params to address this bug.
  2. For consistency and to help avoid future bugs, I decided to specify both type params across all components, regardless of whether they have custom options or not.

While doing the work, I noticed that a few components that had special option types weren't using those in their registry code. So, I added those.

Documentation has been updated to call out this scenario.

📑 Test Plan

Added some code to the React wrapper tests to ensure that the types flow correctly and React components with all properties are generated.

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

There are several follow-ups needed:

  • The same set of changes need to be applied to the FluentUI Web Components
  • I'd like to follow back up at some point and see if I can come up with a nicer alternative to compose that does the same thing under the hood. We'd keep the original, but deprecate it, and update code and docs for the new API.
  • I noticed a few new components that either haven't shown up in our docs and/or haven't shown up in the component explorer. I think we should fix that. The list is avatar, calendar, picker, and search.

@EisenbergEffect
Copy link
Contributor Author

@janechu Any idea what's going on here with the schema generation error that's showing up?

@EisenbergEffect
Copy link
Contributor Author

Hey folks. This really needs reviews because we can't make the React wrappers work in TypeScript with our components without this being merged. /cc @chrisdholt @nicholasrice

@chrisdholt
Copy link
Member

@EisenbergEffect is this still a needed PR? Assuming so, but want to confirm with you.

@EisenbergEffect
Copy link
Contributor Author

@EisenbergEffect is this still a needed PR? Assuming so, but want to confirm with you.

Yep. Without this, the React wrapper can't infer the types properly and the resultant React component is missing properties. We'll need to make a similar update to our Fluent components as well.

@chrisdholt
Copy link
Member

@EisenbergEffect is this still a needed PR? Assuming so, but want to confirm with you.

Yep. Without this, the React wrapper can't infer the types properly and the resultant React component is missing properties. We'll need to make a similar update to our Fluent components as well.

Sounds good - looks like there are some issues with the build regarding component schemas, which is odd. We can take a look at this in the next day or so to figure out how to get this unblocked.

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the warning:stale No recent activity within a reasonable amount of time label Apr 16, 2022
@chrisdholt
Copy link
Member

Closing this as we've removed fast-components

@chrisdholt chrisdholt closed this May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
warning:stale No recent activity within a reasonable amount of time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: Incomplete component attribute type information when using fast-react-wrapper
3 participants