Skip to content

Conversation

@seanemmer
Copy link
Contributor

Update return type from React.ReactNode to JSX.Element

Resolves #138

@seanemmer
Copy link
Contributor Author

This passes the test suite locally - looks like 'yarn build' is failing in the CI pipeline.

@jhuleatt
Copy link
Collaborator

yarn build runs the TypeScript compiler, and it looks like it is failing on a type error caused by the switch to JSX.Element:

auth/index.tsx:85:5 - error TS2322: Type 'ReactNode' is not assignable to type 'Element'.
  Type 'string' is not assignable to type 'Element'.
85     return fallback;
       ~~~~~~~~~~~~~~~~
auth/index.tsx:87:5 - error TS2322: Type 'ReactNode' is not assignable to type 'Element'.
  Type 'string' is not assignable to type 'Element'.
87     return children;
       ~~~~~~~~~~~~~~~~
Found 2 errors.
error Command failed with exit code 2.

I also got this error when running yarn build locally. I'll add some comments in the review that explain how to fix it.

children,
requiredClaims
}: AuthCheckProps): React.ReactNode {
}: AuthCheckProps): JSX.Element {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and the change on line 36 cause a typescript compile error because they conflict with the props declared on line 47 and 48

@jhuleatt
Copy link
Collaborator

After playing around with the PR branch for a bit, I think this confirms that switching to JSX.Element is not the right way to go. I'm still getting warnings when I try to use a string as a child/fallback:

AuthCheck child string error

@seanemmer
Copy link
Contributor Author

seanemmer commented Oct 21, 2019

I wrapped the child/fallback in a React fragment to handle the string scenario - this is similar to what was already done for PerfCheck

@jhuleatt
Copy link
Collaborator

jhuleatt commented Nov 5, 2019

Thank you @seanemmer! Looks like it is working now 🔥

@jhuleatt jhuleatt merged commit d96bda4 into FirebaseExtended:master Nov 5, 2019
@FirebaseExtended FirebaseExtended locked and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AuthCheck is defined as ReactNode rather than JSX.Element

2 participants