Remove Directive type from createInfiniteScroll

[onx2]

I'm getting an error in a project of mine which has a custom component. This component accepts a ref essentially as ref?: Ref<HTMLSpanElement> (other details omitted for brevity).

When consuming the component and passing in the loadingRef from this primitive I get this error

If I'm understanding the intent of the Directive return type here, it really is just a ref - this is correct? If this approach isn't preferred, does anyone have a suggestion on how I can accommodate for Directive and Ref?

[changeset-bot]

⚠️ No Changeset found

Latest commit: e01d494

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[thetarnav]

Don’t use Ref, but a maybe a better type could be inlined there instead.

[onx2]

Ref is the type exposed from solid for ref, I'm not following what you mean. Do you mind elaborating?

// How `Directive` type is created in the signal, meets constraints of `Ref` below
let add: (el: Element) => void = noop;


/**
 * Type of `props.ref`, for use in `Component` or `props` typing.
 *
 * @example Component<{ref: Ref<Element>}>
 */
export type Ref<T> = T | ((val: T) => void);
``

[thetarnav]

forgot solid now exports Ref lol
but I mean this: (el: Element) => void
Instead of using aliases like Directive or Ref

[onx2]

I see - yes that seems to work.

Update: I had a ts-ignore in one place so I was mislead. The error still persists and I think is because Directive expects a props argument but that isn't the case for Ref or the inline type you suggested and props aren't being used. I think they can be removed and we can use the inline or Ref in the component to resolve this issue.

For reference this is what I'm doing now with the updated type def you suggested:

Component

/**
 * Using these extra types allows the generic type passed into `Ref` to be narrowed
 * based on the variant that the consumer passes in.
 */
type HeadingVariantProps = {
  variant: "heading" | "subheading" | "title" | "subtitle";
  ref?: HTMLHeadingElement | ((el: HTMLHeadingElement) => void);
};

type BodyVariantProps = {
  variant: "body";
  ref?: HTMLParagraphElement | ((el: HTMLParagraphElement) => void);
};

type DefaultVariantProps = {
  variant?: undefined;
  ref?: HTMLSpanElement | ((el: HTMLSpanElement) => void);
};

export type TypographyProps = ParentProps & {
  color?: keyof Theme["colors"];
  spacing?: Theme["spacing"];
  wrap?: boolean;
} & (HeadingVariantProps | BodyVariantProps | DefaultVariantProps);

Consumer with loader

<Typography ref={loaderRef}>Loading...</Typography>

TS Error

Type 'Directive' is not assignable to type 'HTMLHeadingElement | ((el: HTMLHeadingElement) => void) | HTMLParagraphElement | ((el: HTMLParagraphElement) => void) | HTMLSpanElement | ((el: HTMLSpanElement) => void) | undefined'.
  Type 'Directive' is not assignable to type '(el: HTMLHeadingElement) => void'.
    Target signature provides too few arguments. Expected 2 or more, but got 1.

[thetarnav]

I meant that createInfiniteScroll should return (el: Element) => void instead of Directive or Ref as it is neither a directive nor a ref, it should just be able to be used with those. Using Ref in your component is fine, thats what it was made for.

[onx2]

I meant that createInfiniteScroll should return (el: Element) => void instead of Directive or Ref as it is neither a directive nor a ref, it should just be able to be used with those. Using Ref in your component is fine, thats what it was made for.

Understood 👍🏼 I can update this PR to make that change. Thank you for talking through this with me.

@thetarnav updated!

[onx2]

@thetarnav Does this look okay now to merge?