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

withAuthUser() HOC prevents Fast Refreshing / HMR #163

Closed
timfee opened this issue May 6, 2021 · 6 comments
Closed

withAuthUser() HOC prevents Fast Refreshing / HMR #163

timfee opened this issue May 6, 2021 · 6 comments
Assignees
Labels
bug Something isn't working v1 Change intended to land in v1.x

Comments

@timfee
Copy link

timfee commented May 6, 2021

Description

When editing a Component that is wrapped with withAuthUser({}), React performs full-page refreshes; for example:

export default withAuthUser({
  whenAuthed: AuthAction.REDIRECT_TO_APP,
  whenUnauthedBeforeInit: AuthAction.RETURN_NULL,
  whenUnauthedAfterInit: AuthAction.RENDER,
})(Auth)

Version

Using the example's version: "0.13.0-alpha.4"

To Reproduce

  1. Clone the gladly-team/next-firebase-auth repo as is
  2. yarn dev in the examples directory
  3. Navigate to auth.js, doesn't matter if you're signed in or not.
  4. Make a trivial change (it doesn't actually even need to cause the DOM to get re-rendered).
  5. Observe that the entire page refreshes.

Additional context

The warning in the console:

[Fast Refresh] performing full reload

Fast Refresh will perform a full reload when you edit a file that's imported by modules outside of the React rendering tree.
You might have a file which exports a React component but also exports a value that is imported by a non-React component file.
Consider migrating the non-React component export to a separate file and importing it into both files.

It is also possible the parent component of the component you edited is a class component, which disables Fast Refresh.
Fast Refresh requires at least one parent function component in your React tree.

Expected behavior

  • HMR works :)

If this is unavoidable without additional configuration, it'd be awesome to capture the necessary steps somewhere in the example or in the doc. And if it's totally unavoidable, it might be a good FAQ note :)

@kmjennison kmjennison added enhancement New feature or request help wanted Extra attention is needed labels May 7, 2021
@kmjennison
Copy link
Contributor

I confirmed this problem doesn't happen if you remove withAuthUser from the page.

I tried breaking useAuthUser into separate modules in case the two exports (the context provider and useAuthUser hook) caused this issue, but that didn't fix it.

Very much welcome help on this!

@timfee
Copy link
Author

timfee commented May 9, 2021

So this is interesting. HMR seems to work for my project I'm tinkering with: https://github.com/timfee/slot.fyi.

Highlights:

  • my app.tsx is wrapped as such:
       <Layout>
          <Component {...pageProps} />
        </Layout>
  • layout.tsx is where I wrapped the HOC
import { FC } from 'react'
import { useAuthUser, withAuthUser } from 'next-firebase-auth'

const Layout: FC = ({ children }) => {
  // snip

  return (
    <>
            { /* snip */ }
            {!auth.firebaseUser && auth.clientInitialized && (
              <button className="px-3 py-2 bg-yellow-300 rounded">
                Sign in
              </button>
            )}
      <main className="max-w-4xl px-6 mx-auto mt-8">{children}</main>
    </>
  )
}

export default withAuthUser({})(Layout)
  • an actual inner page is:
import { Profile } from '@/components/settings/account/Profile'
import { CalendarList } from '@/components/settings/calendars/CalendarList'
import { useCollection } from '@/hooks/useCollection'
import { useDocument } from '@/hooks/useDocument'
import firebase from '@/lib/firebase'
import { UserType } from '@/types/user'
import {
  AuthAction,
  getFirebaseAdmin,
  useAuthUser,
  withAuthUser,
  withAuthUserTokenSSR
} from 'next-firebase-auth'

import { FC } from 'react'

const Account: FC<any> = ({
  meta
}: {
  meta: {
    uid: string
    displayName: string
    emailAddress: string
    photoUrl: string
  }
}) => {
  const authUser = useAuthUser()
  // snip

  return (
    <>
      <Profile
        data={typeof profileData === 'undefined' ? null : profileData.data()}
      />
    </>
  )
}

export const getServerSideProps = withAuthUserTokenSSR({
  whenUnauthed: AuthAction.REDIRECT_TO_LOGIN
})(async ({ AuthUser }) => {
  const UserRecord: UserType = (
    await getFirebaseAdmin().firestore().doc(`/users/${AuthUser.id}`).get()
  ).data()

  console.log(UserRecord)
  const {
    uid,
    meta: { displayName, emailAddress, photoUrl }
  } = UserRecord

  return {
    props: {
      meta: {
        uid,
        displayName,
        emailAddress,
        photoUrl
      }
    }
  }
})

export default withAuthUser({})(Account)

I have no idea why this works, but wanted to share.

@Izhaki
Copy link
Contributor

Izhaki commented Jun 10, 2021

Spent 5 hours on this, and while I don't have a solution, I think I've homed in on the issue (and it's a mega issue if I'm correct)...

One of the oddest issues ever

Basically, if you reduce withAuthUser to this minimal function:

const withAuthUser = ({} = {}) => (ChildComponent) => {
  /* 1 */ const WithAuthUserHOC = (props) => <ChildComponent {...props} /> // Doesn't work
  /* 2 */ const WithAuthUserHOC = ChildComponent // Works

  return WithAuthUserHOC
}

Then Fast Refresh does not work with /* 1 */, but it does with /* 2 */.

This is super odd. So odd, that my immediate suspicion was that something's wrong with the build/transpilation.

So I've put webpack in development mode in order to see what the transpiled code under build looks like and boom... /* 1 */ was suddenly working.

Now this is the first library I see that exports a node distribution (specifically of React components) the way this one does (using webpack). build/index.node.js is littered with webpack stuff that is completely unnecessary - either babel alone or rollup would normally be used in these cases (and we should also mention esm distributions rather than CommonJS, specifically if this is a library for NextJS, which uses WebPack where esm has many benefits).

Anyhow, if indeed someone confirms that Fast Refresh works when webpack is in development mode, I strongly suspect there is something wrong with the way this library is built for node.

And another one

Also part of the investigation, I had NextJs in example building the library code directly. When I did this, I got the blue NextJS warning that isClientSide is not Fast Refresh compliant (It does have an unnamed default export: export default () => typeof window !== 'undefined').

@Izhaki
Copy link
Contributor

Izhaki commented Jun 14, 2021

OK. I know what the issue is.

It is the fact that the library is split distributed with client methods under "browser" in package.json and server under "main". This seems to confuse fast refresh (my theory is that when it tries to replace the modified file it sends to the client the server version of the file, but it can't find it on the client because the client module is not in the same format).

What I've done:

  • Remove the webpack builds for both client and server.
  • Created a single esm build.
  • Commented out any server related exports (only testing /auth page).
  • Fast refresh works.

I'm going to create a new issue regarding how this library is built, with a proposal for a "uni-build", this will have many benefits, but it'll be a breaking change (change to import location for any server related functions).

@kmjennison kmjennison added bug Something isn't working and removed enhancement New feature or request labels Aug 3, 2021
@kmjennison kmjennison mentioned this issue Aug 7, 2021
24 tasks
@yagudaev
Copy link

yagudaev commented Aug 1, 2022

@Izhaki I've actually hit the same issue without even using this library.

As you mentioned, anytime you use a HOC on a next.js page it doesn't work. Here is a quick demo: https://stackblitz.com/edit/nextjs-77e7qw?file=pages/index.js

Any other thoughts or updates? Is this NextJS issue? Or there is a different way to a user be authenticated that might work better?

@kmjennison kmjennison added v1 Change intended to land in v1.x and removed help wanted Extra attention is needed labels Aug 25, 2022
@kmjennison kmjennison self-assigned this Aug 25, 2022
kmjennison added a commit that referenced this issue Aug 26, 2022
kmjennison added a commit that referenced this issue Aug 26, 2022
This was referenced Aug 26, 2022
@kmjennison
Copy link
Contributor

This bug appears to be fixed on v1.x with peer dependencies:

"firebase": "^9.9.1",
"firebase-admin": "^11.0.0",
"next": "12.2.3",
"react": "18.2.0",
"react-dom": "18.2.0",

I attempted to reproduce the problem here, but HMR is working as expected:
#550

It was fixed in v1.0.0-canary.12, possibly because of changing index.server.js to not import from index.js in #512—but we have not investigated the underlying reason. (Unintentional bug fix? We'll take it!)

The bug persists on v0.x:
#551

Closing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v1 Change intended to land in v1.x
Projects
None yet
Development

No branches or pull requests

4 participants