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

Throw if useAuthUser is used without withAuthUser #155

Closed
Tracked by #265
kmjennison opened this issue May 1, 2021 · 7 comments
Closed
Tracked by #265

Throw if useAuthUser is used without withAuthUser #155

kmjennison opened this issue May 1, 2021 · 7 comments
Assignees
Labels
breaking The issue or PR will introduce a breaking change enhancement New feature or request help wanted Extra attention is needed

Comments

@kmjennison
Copy link
Contributor

Is your feature request related to a problem? Please describe.
If a developer accidentally uses useAuthUser without withAuthUser, it won't work: useAuthUser will always return an unauthed user. This is confusing: #150

Describe the solution you'd like and how you'd implement it
Throw an error if useAuthUser is called without withAuthUser above it in the component tree.

Implementation: don't set a default AuthUser value when defining the context, and in the useAuthUser function, throw if the value is undefined.

Is this a breaking change?
Yes, somewhat. If a developer is using this incorrectly, upgrading would throw a new error.

Describe alternatives you've considered

  • Leave as-is, though I don't see the value of useAuthUser without a context provider
  • Warn rather than throw
@kmjennison kmjennison added enhancement New feature or request help wanted Extra attention is needed breaking The issue or PR will introduce a breaking change labels May 1, 2021
@kmjennison kmjennison mentioned this issue Aug 7, 2021
24 tasks
@lazlothemonkey
Copy link

lazlothemonkey commented Sep 3, 2021

I have a header component which renders different UI whether or not AuthUser is defined or not. For example if AuthUser exists, the header will have a logout button with the handling logic within that Header component. This header component is imported in every page which needs it.

Now in this case, I want to use UseAuthUser directly in the Header component, however since I want to use the Header component also on pages without the withAuthUser HOC (for example the welcome page), this is a case where I actually use useAuthUser in a component (the header) without the withAuthUser HOC for the page which imports this component. And the useAuthUser in this case returning unauthed User would be intended, because I would render a different UI for that case...

In the example website, I see that the header component receives the AuthUser and Sign out handler function through props... Why wouldn't it be better to just use useAuthUser directly in the header?

@kmjennison
Copy link
Contributor Author

@lazlothemonkey Good question. I'd recommend one of the following approaches for your header scenario, depending on what you need:

  1. Use withAuthUser on all pages. The only overhead here is loading the Firebase JS SDK.
  2. Pass the user info as a prop to your header component from each page that has withAuthUser, like the example app does. (There's no real reason the example app does it this way. I just decided it would be a presentational component.)
  3. Create separate header components for pages with and without authed user info. You already know in advance which pages use which, because they're the ones using withAuthUser.

@lazlothemonkey
Copy link

Thanks, I think I'll go with option 3, so that I don't have one huge component with an if else block containing two different UIs

@kmjennison
Copy link
Contributor Author

Closed in #395.

@adithep
Copy link

adithep commented Feb 8, 2022

@kmjennison What about static hoisting?

#237

Currently it throws even if you are using getLayout pattern:

https://nextjs.org/docs/basic-features/layouts#per-page-layouts

@iisalazar
Copy link

Was this issue resolved? I'm currently facing it right now

@kmjennison
Copy link
Contributor Author

@adithep Could you clarify what you mean? If you believe there's a bug, please open a new bug issue with a reproduction. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking The issue or PR will introduce a breaking change enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants