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.

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

feat: upgrade to react 18 - fix build #2 #3774

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

neatbyte-vnobis
Copy link
Collaborator

Changes

First batch of fixes for project build after updating to react 18.
https://www.notion.so/webiny/Upgrade-to-React-18-ceacd9bbfcf74b39a7ceb70943c3afa7

Packages: app-cognito-authenticator, react-router, ui

How Has This Been Tested?

Manually.

Documentation

None.

@neatbyte-ivelychko
Copy link

@brunozoric

While working on this batch I have encountered some issue with build for @webiny/ui.
Error while building @webiny/ui: it fails in this file node_modules/@rmwc/base/dist/component.d.ts because function createComponent from @rmwc is using type React.RefForwardingComponent that has been removed from React 18. Maybe this issue could be fixed by updating rmwc to the latest version? I have checked their website and I saw that they use React 18 in the latest version of the library.

@brunozoric
Copy link
Contributor

@neatbyte-ivelychko

Yes, you can update the rmwc. Probably make it as separate PR because there will be a lot of changes.

@@ -15,24 +16,29 @@ export interface RequireNewPassword {
requiredAttributes: string[];
}

type CheckContactParams = {
verified: Record<any, any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please figure out some better types. We cannot trust what you currently wrote.

@@ -21,6 +22,11 @@ interface Reducer {
(prev: State, next: Partial<State>): State;
}

type CheckContactParams = {
verified: Record<any, any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please figure out better types.

Choose a reason for hiding this comment

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

@brunozoric improved Record<any, any> type

Copy link
Contributor

Choose a reason for hiding this comment

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

@neatbyte-ivelychko putting object to AuthData interface instead of Record<any, any> is same bad type written in another way.
The verified is not used anywhere except two checks if it actually is defined, so maybe try to figure out what is contained in it? Don't spend much more time on it. If you can't find the correct type, put

{verified?: Record<string, string | number | boolean>}

for now.

Also, I see that unverified is not used anywhere, so maybe it is not necesary at the moment...?

Choose a reason for hiding this comment

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

But still we need to add object in the AuthData type [key: string]: string | null | boolean | undefined | object; property. Becuase TS keeps arguing that that type does not support object, it only supports string | null | boolean | undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

When logging the data variable, I get:

{
    "verified": {
        "email": "my@email.com"
    },
    "unverified": {}
}

Maybe it could be like this?

export interface AuthDataVerified {
    email?: string;
}

export interface AuthDataUnverified {
    someUnknownKey?: string;
}

export interface AuthData {
    username?: string;
    email?: string;
    verified?: AuthDataVerified;
    unverified?: AuthDataUnverified;
    [key: string]: string | null | boolean | undefined | AuthDataVerified | AuthDataUnverified;
}

Choose a reason for hiding this comment

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

Okay, I have also checked method verifiedContact in @aws-amplify and I saw that besides email property they have phone_number property. Should we consider adding it to the verified and unverified.

if (attrs['email']) {
   if (attrs['email_verified']) {
	verified['email'] = attrs['email'];
   } else {
	unverified['email'] = attrs['email'];
   }
}
 if (attrs['phone_number']) {
   if (attrs['phone_number_verified']) {
	verified['phone_number'] = attrs['phone_number'];
    } else {
      unverified['phone_number'] = attrs['phone_number'];
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@neatbyte-ivelychko yes, add anything you can find. Maybe we don't use those vars now, but let's have them in types.

Choose a reason for hiding this comment

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

Added phone_number to the type

Choose a reason for hiding this comment

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

@brunozoric I think that this might be the latest batch. Because after I have fixed type for ComposableFC project had been built without any errors.

We just need to check whether it is gonna pass CI/CD.

Choose a reason for hiding this comment

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

Nevermind, it was the issue with my local build. Somehow it was always green event if build had errors

@brunozoric brunozoric merged commit 886860a into bruno/feat/upgrade-to-react-18 Dec 21, 2023
11 of 13 checks passed
brunozoric pushed a commit that referenced this pull request Jan 11, 2024
brunozoric pushed a commit that referenced this pull request Jan 12, 2024
brunozoric pushed a commit that referenced this pull request Jan 17, 2024
brunozoric pushed a commit that referenced this pull request Feb 6, 2024
brunozoric pushed a commit that referenced this pull request Feb 14, 2024
brunozoric pushed a commit that referenced this pull request Mar 13, 2024
brunozoric pushed a commit that referenced this pull request Mar 18, 2024
brunozoric pushed a commit that referenced this pull request Mar 28, 2024
brunozoric pushed a commit that referenced this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants