Add types for specific fields types.

maslade commented 5 years ago

Is your feature request related to a problem? Please describe.

The Text component expects a field of type { value?: string, editable?: string}, but it doesn't export a definition for this, making it difficult to work with. If I use the base Field type provided in PlaceholderProps, the compiler reports the following:

[ts]
Type 'Field | Item[]' is not assignable to type '{ value?: string | undefined; editable?: string | undefined; }'.
  Type 'Field' is not assignable to type '{ value?: string | undefined; editable?: string | undefined; }'.
    Types of property 'value' are incompatible.
      Type 'string | number | boolean | { [key: string]: any; } | { [key: string]: any; }[]' is not assignable to type 'string | undefined'.
        Type 'number' is not assignable to type 'string | undefined'. [2322]

Describe the solution you'd like

I would like Sitecore to provide strong types for each of the common field types defined in sitecore-jss-manifest, and I'd like the components defined in sitecore-jss-react to leverage those. For example, a SingleLineText field type that would match the signature that the Text component expects in its field prop.

Alternatively, if that cross-package dependency doesn't make sense for JSS, I'd like the React components to export their own version of these types so I can leverage those.

Describe alternatives you've considered

My current solution is to extend the Field type from sitecore-jss as follows:

export interface ITextField extends Field {
  value: string;
  editable?: string;
}

aweber1 commented 5 years ago

Would you be able to provide a code sample that illustrates how the current TextProps interface is difficult to work with in the context of the Text component? Not trying to be obtuse, just trying to better understand the issue.

maslade commented 5 years ago

I just did some digging and learned that I can reference the type of the field property using TextProps['field'], so the following works:

interface ComponentProps { fields: { heading: TextProps['field']; } }

Closing.

maslade commented 5 years ago

Sorry, I was too hasty. Reopening as this introduces a related problem. Considering the following POC:

import { Text } from '@sitecore-jss/sitecore-jss-react';
import { PlaceholderProps } from '@sitecore-jss/sitecore-jss-react/types/components/PlaceholderCommon';
import { TextProps } from '@sitecore-jss/sitecore-jss-react/types/components/Text';

interface ComponentProps extends PlaceholderProps {
  fields: {
    heading: TextProps['field'];
  }
}

const POCComponent = (props: ComponentProps) => {
  const heading = (props.fields || {}).heading;

  return <Text field={heading} />
}

Using TextProps['field'] fixes type compatibility between the field and the Text component it gets passed into. However this creates a conflict with the PlaceholderProps base interface that I'm extending due to PlaceholderProps['fields'] not matching the type definition for TextProps['field']. The former is based on the Field type that requires a value property; the latter defines its value as optional.

[ts]
Interface 'ComponentProps' incorrectly extends interface 'PlaceholderProps'.
  Types of property 'fields' are incompatible.
    Type '{ heading: { value?: string | undefined; editable?: string | undefined; }; }' is not assignable to type '{ [name: string]: Field | Item[]; }'.
      Property 'heading' is incompatible with index signature.
        Type '{ value?: string | undefined; editable?: string | undefined; }' is not assignable to type 'Field | Item[]'.
          Type '{ value?: string | undefined; editable?: string | undefined; }' is not assignable to type 'Field'.
            Types of property 'value' are incompatible.
              Type 'string | undefined' is not assignable to type 'string | number | boolean | { [key: string]: any; } | { [key: string]: any; }[]'.
                Type 'undefined' is not assignable to type 'string | number | boolean | { [key: string]: any; } | { [key: string]: any; }[]'. [2430]
interface ComponentProps

This is because TextProps['field'] has an optional value property, whereas Field (which PlaceholderProps specifies) defines value as required. This goes away if I choose not to extend PlaceholderProps, but then I lose props.rendering, props.params, and so on.

This could be fixed by defining TextProps.field.value as required, but I'm not sure if that makes sense from a JSS internals standpoint.

kamsar commented 5 years ago

Semantically speaking you should not be extending PlaceholderProps because a component within a placeholder is not a Placeholder component. For example, the missingComponentComponent field on PlaceholderProps has no meaning on a component within said placeholder.

maslade commented 5 years ago

@kamsar I see. Is there a better alternative available? I'd like to avoid having to define my own types that parallel Sitecore internals.

stale[bot] commented 2 years ago

This has been automatically marked as stale because it has not had recent activity. It will be closed if there is no further activity within 30 days. You may add comments or the 'keep' label to prevent it from closing. Thank you for your contributions.

Sitecore / jss