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

[hawkticehurst]

🐛 Bug Report

Hi, over on the Webview UI Toolkit we're hoping to ship a set of React components using the fast-react-wrapper.

As we've been working towards this we've run into an issue where wrapped toolkit components do not have any type annotations/information for any of their attributes resulting in TypeScript intellisense/build errors.

We've also had some people from our community try out the fast-react-wrapper all with the same results.

I'm still not completely sure if this is an issue with the fast-react-wrapper itself or how the toolkit is implemented (I suspect it might actually be the latter since the toolkit is closely modeled after the FAST Components repo and that doesn't have the same issue).

Any help in determining what the issue is and how to fix it would be greatly appreciated! Thanks!

💻 Repro or Code Sample

Here's a simple example of wrapping a <vscode-button> and using it in a counter component.

// Inside toolkit.ts file

import React from "react";
import { provideReactWrapper } from "@microsoft/fast-react-wrapper";
import { provideVSCodeDesignSystem, vsCodeButton } from "@vscode/webview-ui-toolkit";

const { wrap } = provideReactWrapper(React, provideVSCodeDesignSystem());

// Toolkit Components
export const VSCodeButton = wrap(vsCodeButton());

// Inside Counter.tsx file

import { useState } from "react";
import { VSCodeButton } from "./toolkit";

export function Counter() {
  const [count, setCount] = useState(0);

  function incrementCount() {
    setCount(count + 1);
  }

  // ❗️❗️ ERROR HERE ❗️❗️
  //
  // The `appearance` attribute and any other vscode-button attributes (both custom and
  // default attributes) will have an intellisense type error and will not build
  //
  // Note: A screenshot of the intellisense/build error can be found below
  return (
    <VSCodeButton appearance="secondary" onClick={incrementCount}>
      The count is: {count}
    </VSCodeButton>
  ); 
}

🤔 Expected Behavior

Wrapped components should have complete/correct type annotations for all of their attributes.

😯 Current Behavior

Here's a simple example of a component attribute and the associated error message:

💁 Possible Solution

🔦 Context

Our hope is to ship a set of React components from the Webview UI Toolkit package.

🌍 Your Environment

• OS & Device: MacOS on MacBook Pro
• Browser: VS Code Webview (Chromium-based iirc?)
• Version: FAST Element (v1.6.0) & FAST Foundation (v2.21.0)

[hawkticehurst]

I've found a bit of time in the last day to do a bit of my own work to see if I can identify what's going on with the components and finally just discovered something that may (?) be helpful in identifying whatever bug is causing this issue.

Culprit: {Component}Options?

I've found that components that take {Component}Options as a type generic in the compose method seem to be the components that cannot be compiled and have incorrect intellisense once wrapped with fast-react-wrapper. For example, the below code (once wrapped as a React component) will not be compiled in a TypeScript-based React application.

import { ButtonOptions } from '@microsoft/fast-foundation';

// ... other component code ...

export const vsCodeButton = Button.compose<ButtonOptions>({
	baseName: 'button',
	template,
	styles,
	shadowOptions: {
		delegatesFocus: true,
	},
});

Looking at this from the perspective of generated type annotations, any components that have {Component}Options will not have typeof {Component} included as part of its type declaration.

By removing the {Component}Options code, like so...

// ... other component code ...

export const vsCodeButton = Button.compose({
	baseName: 'button',
	template,
	styles,
	shadowOptions: {
		delegatesFocus: true,
	},
});

...you will get a generated type annotation that does include typeof {Component}. In the case of the VS Code button, by removing ButtonOptions it will result in the following change.

WITH ButtonOptions:

WITHOUT ButtonOptions:

Lingering issues

With this new type annotation, the VSCodeButton React component will actually compile and render (🎉), however, Intellisense will still complain about the attribute.

Removing {Component}Options isn't the solution

Finally, removing {Component}Options isn't a full-proof solution either (despite my efforts to test it) because as I'm sure you know it's needed for defining things like custom icons within components. So without, say, CheckboxOptions I'll get TS intellisense errors when building the toolkit components.

I hope this is helpful and saves some time in assessing and identifying the bug at hand. I'll also keep working on this and report back with any new findings.

[EisenbergEffect]

I see what's happening in the type system now. Let's see if I can figure out how to fix it. This is going to be a fun one 😉

[EisenbergEffect]

Ok, I've got a PR up that fixes our own components: #5473

Basically, there appears to be a limitation in TS type inference. So, you can update how your are calling compose and that should fix things for you.

DO THIS

const myComponent = MyComponent.compose(...);

OR DO THIS

const myComponent = MyComponent.compose<MyComponentOptions, typeof MyComponent>(...);

BUT DO NOT DO THIS

const myComponent = MyComponent.compose<MyComponentOptions>(...);

It appears that explicitly adding the first type param prevents TS from inferring the second one. So, no params is fine because it can infer everything. But if you add the options param, you need to also add the second param. In our components, I've put together a PR that just has everything specify both for consistency. That would be my recommendation, lest someone copy/paste one without both, add the options, and then produce something that can't be properly wrapped (due to missing type info).

Hopefully that all makes sense. Let me know how it works out for you.

[EisenbergEffect]

NOTE: I'm also thinking about creating a new API that makes this more ergonomic in light of this TS limitation. Either way, we won't remove the old one until a major version change. We just might switch over to using a new thing and update docs accordingly. Not sure if I'll get around this soon or not.

[hawkticehurst]

Woo, thank you so much! That definitely makes sense (although it's quite the interesting quirk 🧐).

I'll try to draft up my own PR tomorrow and report back on the results.

[hawkticehurst]

This fixed the issue!!

With that said, I'm finding there is one more lingering problem: some fast foundation component attribute value types are defined with enums (i.e. TextFieldTypes, SelectPosition, etc.) which are currently inaccessible to toolkit users and results in TS errors.

An easy solution is to import/export these enums so they are consumable by those using the toolkit––however this would still result in what I think is a bit of a clunky/unintuitive developer experience:

// This does NOT work (i.e. TS Error: Type '"password"' is not assignable to type 'TextFieldType | undefined'.)
<VSCodeTextField type="password"></VSCodeTextField>

// This works but is a bit clunky imo
import {TextFieldTypes} from "toolkit";
<VSCodeTextField type={TextFieldTypes.password}></VSCodeTextField>

As a result, I'm wondering if you might be open to providing or even converting instances of these enums into string literal union types within fast foundation components so that there is still type safety/restrictions, but end users can simply type out the literal value versus being forced to import and use the enums?

For example:

// Instead of this
export enum TextFieldType {
    email = "email",
    password = "password",
    tel = "tel",
    text = "text",
    url = "url",
}

// Create and use this
export type TextFieldType = "email" | "password" | "tel" | "text" | "url";

Let me know what you think!

[EisenbergEffect]

I'm definitely open to that. We could start by exporting the existing enum to get things unblocked. Maybe we can provide some sort of typing that enables using either the enum or the string literal? I haven't tried that with TS. Basically, I want to see if we can avoid a breaking change. @chrisdholt Any thoughts?

[EisenbergEffect]

Actually, @hawkticehurst can you open a new issues for this request? I'm going to close this one out since the original issue is fixed. Then we can discuss the enums in a fresh issue.

[hawkticehurst]

Yep, would be happy to!