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.

Sign up for GitHub

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

[TS migration] Migrate 'FormElement.js' component to TypeScript #31012

Merged
merged 13 commits into from
Nov 16, 2023
Merged

[TS migration] Migrate 'FormElement.js' component to TypeScript #31012

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
78a09b7
ref: moved FormElement to TS
kubabutkiewicz Oct 16, 2023
af566d8
Merge branch 'main' of github.com:kubabutkiewicz/expensify-app into t…
kubabutkiewicz Nov 6, 2023
d340487
fix: typing
kubabutkiewicz Nov 7, 2023
b52fbb3
Merge branch 'main' of github.com:kubabutkiewicz/expensify-app into t…
kubabutkiewicz Nov 7, 2023
b2dc291
fix: changed a type
kubabutkiewicz Nov 7, 2023
7e7602f
Revert "ref: moved FormElement to TS"
kubabutkiewicz Nov 8, 2023
1d8c3a3
Merge branch 'main' of github.com:kubabutkiewicz/expensify-app into t…
kubabutkiewicz Nov 8, 2023
5578518
Revert "ref: moved FormElement to TS"
kubabutkiewicz Nov 8, 2023
f3133cb
fix: removed unnecessary type
kubabutkiewicz Nov 8, 2023
30a3043
fix: changed dispalyName of component
kubabutkiewicz Nov 9, 2023
cadb376
Merge branch 'main' of github.com:kubabutkiewicz/expensify-app into t…
kubabutkiewicz Nov 13, 2023
627b85a
fix: resolve comments
kubabutkiewicz Nov 15, 2023
2571645
Merge branch 'main' of github.com:kubabutkiewicz/expensify-app into t…
kubabutkiewicz Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions src/components/FormElement.js
View file
Open in desktop

This file was deleted.

18 changes: 18 additions & 0 deletions src/components/FormElement.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React, {ForwardedRef, forwardRef} from 'react';
import {View, ViewProps} from 'react-native';
import * as ComponentUtils from '@libs/ComponentUtils';

function FormElement(props: ViewProps, ref: ForwardedRef<View | HTMLFormElement | null>) {
return (
<View
role={ComponentUtils.ACCESSIBILITY_ROLE_FORM}
ref={ref as ForwardedRef<View>}
Copy link
Contributor

@barttom barttom Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

still don't get it why You need assertion here and why this assertion is different then defined ref in property. Could You explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure , here which is SignInFormPage for web platform
image
we are passing ref of type HTMLFormElement
but in FormElement we are using View where View expect ref to be a type of View
image
Thats why we I need to assert it

Copy link
Contributor

Choose a reason for hiding this comment

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

The typing situation here is quite terrible. In React Native for Web, View component with role={ComponentUtils.ACCESSIBILITY_ROLE_FORM} will be backed by a HTMLFormElement (which we can access through the ref), while neither HTMLFormElement is a subtype of View nor the other way around. In runtime, though, if I remember correctly, React Native for Web will patch the returned element, so in practice we could say that it does conform to the View API.

It doesn't help that in TypeScript Object1 is assignable to Object2 in such scenario:

interface Object1 {
    prop1: boolean;
}

interface Object2 {
    prop1: string | number;
}

...which is fundamentally unsound, as we can put a 123 inside prop1 of Object1 through an Object2 reference without a single type assertion. I wasn't fully aware about this until today.

This adds another layer of a mess to our situation, as you should't even be allowed to pass MutableRefObject<HTMLFormElement> to ForwardRef<HTMLFormElement | View>.

I won't even start about how most refs don't even end-up being mutable, although they always should be, type-wise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having said all that, I think we add even more terrible typing to a pile of terrible typing.

I would just stick with ForwardRef<View> in FormElement and cast once on the use-site (SignInPageForm):

    const formViewRef = useRef<View | null>(null);

    useEffect(() => {
        const formViewCurrent = formViewRef.current;

        if (!formViewCurrent) {
            return;
        }

        const formCurrent = formViewCurrent as unknown as HTMLFormElement;
        
        // ...
    });

And yes, the as unknown is necessary, because this is a terrible cast. We just were hiding it from the type system before. And yes, it's probably worth commenting.


Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we could remove the "force cast" (as unknown as...) by using a slightly bizarre cast...

const formCurrent = formViewCurrent as (HTMLFormElement & View);

...as HTMLFormElement & View is, actually, the most precise type for what this object should be in runtime on React Native for Web.

// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
/>
);
}

FormElement.displayName = 'BaseForm';
Copy link
Contributor

@barttom barttom Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is it a proper displayName? Doesn't follow file name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was here before so I am not sure if this should be changed in TS migration

Copy link
Contributor

Choose a reason for hiding this comment

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

If you take a look at blame here they must've missed it.

Let's change it to 'FormElement' to follow the guidelines, other than that LGTM!


export default forwardRef(FormElement);