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 5 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
10 changes: 5 additions & 5 deletions ios/Podfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,9 @@ PODS:
- nanopb/encode (= 2.30908.0)
- nanopb/decode (2.30908.0)
- nanopb/encode (2.30908.0)
- Onfido (28.3.0)
- onfido-react-native-sdk (8.3.0):
- Onfido (~> 28.3.0)
- Onfido (27.4.0)
- onfido-react-native-sdk (7.4.0):
- Onfido (= 27.4.0)
- React
- OpenSSL-Universal (1.1.1100)
- Plaid (4.1.0)
Expand Down Expand Up @@ -1209,8 +1209,8 @@ SPEC CHECKSUMS:
MapboxMaps: af50ec61a7eb3b032c3f7962c6bd671d93d2a209
MapboxMobileEvents: de50b3a4de180dd129c326e09cd12c8adaaa46d6
nanopb: a0ba3315591a9ae0a16a309ee504766e90db0c96
Onfido: c7d010d9793790d44a07799d9be25aa8e3814ee7
onfido-react-native-sdk: b346a620af5669f9fecb6dc3052314a35a94ad9f
Onfido: e36f284b865adcf99d9c905590a64ac09d4a576b
onfido-react-native-sdk: 4ecde1a97435dcff9f00a878e3f8d1eb14fabbdc
OpenSSL-Universal: ebc357f1e6bc71fa463ccb2fe676756aff50e88c
Plaid: 7d340abeadb46c7aa1a91f896c5b22395a31fcf2
PromisesObjC: 09985d6d70fbe7878040aa746d78236e6946d2ef
Expand Down
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';

type FormElementProps = ViewProps;
function FormElement(props: FormElementProps, 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);