Skip to content

Commit

Permalink
forwardRef
Browse files Browse the repository at this point in the history
  • Loading branch information
@Luiz-Monad @alfonsogarciacaro
Luiz-Monad authored and alfonsogarciacaro committed Jul 23, 2019
1 parent 956ad15 commit fa43005
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions src/Fable.React.fs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type IReactExports =
abstract createElement: comp: obj * props: obj * [<ParamList>] children: ReactElement seq -> ReactElement
abstract createContext: defaultValue: 'T -> IContext<'T>
abstract createRef: initialValue: 'T -> IRefValue<'T>
abstract forwardRef: fn: ('props -> IRefValue<'T> option -> ReactElement) -> ReactElementType<'props>
abstract memo: render: ('props -> ReactElement) * areEqual: ('props -> 'props -> bool) -> ReactElementType<'props>
abstract Fragment: ReactElementType<obj>
abstract Suspense: ReactElementType<obj>
Expand Down

5 comments on commit fa43005

@cmeeren
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luiz-Monad @alfonsogarciacaro Should it really be IRefValue<'T> option? Shouldn't it rather be IRefValue<'T option>? AFAIK you never need IRefValue<'T> option, because it will always exist and have a current property (which may be null, i.e. should be wrapped in option - strictly speaking, it would make most sense for the IRefValue itself to be designed with an option wrapper for current).

Is my understanding correct?

@Luiz-Monad
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the way I'm using it, the ref "pointer" itself can be null, but I assume it can also be an object with the current property null. So I left it to the 'T definition to specify if the RefValue Current is nullable.

    let maskedInputTypeHoc =
        let fn = forwardRef <| fun ( props: ITextMaskFieldProp ) ref ->
            // use ref
 ChildrenProp.InputProps [
            /// Properties applied to the Input component
            ///   inside the TextField.
            InputProp.InputComponent Impl.maskedInputTypeHoc
        ]

Indeed the type itself is val ref : IRefValue<Browser.Types.Element option> option.
Perhaps that's overkill, but I don't trust things coming from JS, they always can be null.

Untitled

@Luiz-Monad
Copy link
Contributor Author

@Luiz-Monad Luiz-Monad commented on fa43005 Aug 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually see forwardRef being used in this way in typescript.

React.forwardRef(
    (props: ComponentProps, ref?: React.Ref<HTMLButtonElement>) =>
          //do something

And it's defined as

    interface RefObject<T> {
        readonly current: T | null;
    }
    type Ref<T> = { bivarianceHack(instance: T | null): void }["bivarianceHack"] | RefObject<T> | null;
    interface RefForwardingComponent<T, P = {}> {
        (props: PropsWithChildren<P>, ref: Ref<T>): ReactElement | null;
       //ommited
    }
    interface RefAttributes<T> extends Attributes {
        ref?: Ref<T>;
    }
    function forwardRef<T, P = {}>(Component: RefForwardingComponent<T, P>): 
      ForwardRefExoticComponent<PropsWithoutRef<P> & RefAttributes<T>>;

It can either be a RefObject of T or null and the current can also be either T or null. (bivariancehack is for inheritance in TS, ignore it). The function returns RefAttributes that matches it with ref?, it receives a nullable ref and returns an object with a ref, and that ref itself can either be nullable or the object that it points to can be nullable.

So I guess that definition is correct, then I preferred to err on being too explicit than getting an undefined error at my face.

it would make most sense for the IRefValue itself to be designed with an option wrapper for current

That would also work but needs changes to IRefValue type, it would more closely match the typescript definition, it sure makes more sense, but I didn't want to change other things. Also, createRef will always return it as non-null.

@cmeeren
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good to get that clarified.

Whether to have current be 'T option instead of 'T is up to @alfonsogarciacaro and other maintainers. AFAIK it would make the most sense, but it would also be a breaking change.

@alfonsogarciacaro
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's true I haven't been always consistent about using F# options to represent nulls in JS but this is not always straightforward so I've my doubts now. Discussed here: fable-compiler/fable-browser#21

Recently I use IRefValue in combination with useEffect and at the point the effect is called is more or less guaranteed (if I'm not mistaken) the ref contains a value, that's why I didn't worry to much about that.

Please sign in to comment.