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

Feature Request: Offer Auth Provider #64

Closed
lukebennett88 opened this issue Feb 9, 2021 · 6 comments
Closed

Feature Request: Offer Auth Provider #64

lukebennett88 opened this issue Feb 9, 2021 · 6 comments
Labels
wontfix This will not be worked on

Comments

@lukebennett88
Copy link

Is your feature request related to a problem? Please describe.
I'm looking at adding next-firebase-auth to a site I'm working on, but it has a persistent nav which shows the login status.

Unless I'm missing something, I would have to wrap every single page with the withAuthUser() HOC, as well as call the useAuthUser function in each of those pages. I would also need to move my Layout component out of my _app and into every page, meaning even more duplication on every page.

Describe the solution you'd like and how you'd implement it
A React Context Provider could be exposed so that developer can wrap their _app component with this allowing them to call the useAuthUser hook from any nested component.

Is this a breaking change?
I'm not sure if this would be a breaking change, I'm pretty sure this could be achieved without breaking the useAuthUser() API, but I haven't tried so I'm not sure.

Describe alternatives you've considered
Since this functionality does appear to exist, the only way I can think to achieve this is to not use next-firebase-auth at all, and instead write the functionality myself.

Thanks for your consideration.

@tlays
Copy link
Contributor

tlays commented Feb 9, 2021

Hi @lukebennett88, it's an interesting request, and a good idea. In one of my projects I had actually built this feature myself base on this example which I had enhanced to lazy-load some components. However in the end, I scrapped all my code and came back to next-firebase-auth (regardless of the "duplication" of withAuthUser()) because it was slowing down the performance of some of my pages.

The biggest problem with nextJs and adding an AuthProvider at the root of your application (in _app) is that it is loaded on all pages, all the time ... which means the full firebase-client (at minimum) module is always loaded (even on pages that don't use/need it). This slows down the performance of your pages (due to the extra kb, and JS processing time). Of course you can always play with lazy-loading modules, but no matter what you do, you still end up with some additional JS bundles. Personally, I like keeping my _app as clean and light as possible.

Now if the app has a persistent login status in a permanent navbar (like yours), it's less of a problem, obviously. But for other sites/apps that don't want to extra bytes loaded on every page, it's a pain.

Now I'm not saying that next-firebase-auth cannot be enhanced to offer an AuthProvider for those that don't mind the extra bytes loaded all the time... I'm just not sure if it's worth it. Maybe a custom-built solution like the one I mentioned above is better for you ? Or you could also build the AuthProvider yourself on top of next-firebase-auth if you think it's worth the time (vs copy/pasting withAuthUser in your pages).

@kmjennison
Copy link
Contributor

@lukebennett88 Thanks for the feature request.

If you're okay with using the Firebase user loaded only on the client side, you can wrap a component in withAuthUser and use that component in the _app.js structure. Then, you should be able to use useAuthUser once in your persistent nav bar component. Does this work in your case?

If you're hoping to include the Firebase user in _app.js during SSR, it's possible using getInitialProps and custom auth code, but I'm inclined to agree with @tlays that it's not a pattern we'd want to encourage because it disables static optimization for the whole app. Instead, I'd recommend creating common HOC and getServerSideProps wrappers for your pages.

Off the top of my head, here's another somewhat hacky approach you could take: in _app.js, pass the "AuthUserSerialized" page prop to the withAuthUser-wrapped component that's also rendered in _app.js. This should support SSR when the page includes withAuthUserTokenSSR but won't disable static optimization for the whole app, and the AuthUser should be available in the persistent nav bar component. (This relies on the next-firebase-auth internal prop name.)

@lukebennett88
Copy link
Author

I still think a provider might be a good thing to offer as some people might need it (but discourage people from using it in the README).

Until Next has support from running getStaticProps etc in _app, or an alternative for statically rendering external data sources, I think people are going to want to at least have the option to use global data, as you can easily end up with quite a lot of boilerplate code that you will otherwise need to add to every page.

Having said that, I've done a bit of thinking and I agree with you @tlays — it's not worth loading up Firebase on every page. I'm going to refactor how my nav works instead.

Thanks so much for your suggestions and detailed responses, I really appreciate it 😊

@samos123
Copy link

I am pretty much in the same boat. I have a component called Layout that is loaded in _app.js. The layout component has the following navigation buttons:

        {AuthUser ? (
          <Button variant="primary" onClick={AuthUser.signOut}>
            Logout
          </Button>
        ) : (
          <SigninButton/>
        )}

I was able to get it working by wrapping the Layout component like this:

export default withAuthUser()(Layout)

@kmjennison
Copy link
Contributor

I'm going to close this and aim to have this package follow Next.js's features/recommendations for data fetching. Right now, Next doesn't support getStaticProps or getServerSideProps in _app.js, though it likely eventually will.

@kmjennison kmjennison added the wontfix This will not be worked on label Feb 10, 2021
@dylangolow
Copy link

dylangolow commented Apr 12, 2021

I found this discussion after seeing a similar issue that @lukebennett88 mentioned with having to copy layouts between components and then add withAuthUser to each default export.

My solution was to have a LayoutWithNav component that gets set like this:

// SomePage.tsx

import LayoutWithNav from "../LayoutWithNav"

const SomePage = () => { ...details using useAuthUser() hook }

SomePage.Layout = LayoutWithNav;

export default withAuthUser()(SomePage);

LayoutWithNav looks like this:

// LayoutWithNav.tsx

const LayoutWithNav: React.FC = ({children}: Props) => {

  return (
          <Wrapper>
              <Nav hide={false} />
              <ContentWrap>
                  <Header />
                  <Content>{children}</Content>
              </ContentWrap>
          </Wrapper>
      );
}

export default withAuthUser()(LayoutWithNav);

This solved my issues so that I can use the useAuthUser() hook anywhere I add that LayoutWithNav to a page. Sharing here in case someone else comes across this issue and could use a concrete example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants