fix(auth): Fix clerk issues with early mounting and unmounting

[zackdotcomputer]

For release notes

This is not a breaking change. Even apps using Clerk don't have to make any changes if they don't want to. But if they do want to take advantage of this fix they need to:

1. Remove their existing implementations of ClerkAuthConsumer and ClerkAuthProvider in App.{js,tsx}
2. Add this new ClerkAuthProvider implementation:

   redwood/packages/cli/src/commands/setup/auth/providers/clerk.js

   Lines 19 to 32 in 9c878be

   	const ClerkAuthProvider = ({ children }) => {
   	const frontendApi = process.env.CLERK_FRONTEND_API_URL
   	if (!frontendApi) {
   	throw new Error('Need to define env variable CLERK_FRONTEND_API_URL')
   	}
   	return (
   	<ClerkProvider frontendApi={frontendApi} navigate={(to) => navigate(to)}>
   	<ClerkLoaded>
   	{children}
   	</ClerkLoaded>
   	</ClerkProvider>
   	)
   	}`,

3. Update their imports in App.{js,tsx} and replace withClerk with ClerkLoaded

Or, they can just the codemod that's in development here: #5969

Original PR Description

---

Motivation

This is in response to an issue reported on the forums where user Isaac noticed that Clerk was causing a React memory leak error every time the user logged out.

The underlying problem was that earlier versions of Clerk deleted their underlying client on logout, which was causing our use of the withClerk HOC to unmount and remount the entire app tree, which was causing a leak when the old instance of AuthProvider tried to respond to the change in app state.

Part of the fix for this was to update clerk to version 3.x for all libraries, which ultimately stopped the issue where the Clerk client would be unloaded. However, this introduced an issue where user state was not changing on log-in. The fix for that required exposing a pathway for AuthClient implementations to notify the AuthProvider in a reacty way that a reauthentication is required.

Changes

• AuthProvider moved to being a FunctionalComponent instead of a class-based one, allowing it to use hooks to trigger updates.
• authClients/index broken up into authClients/AuthClient.ts and authClients/SupportedAuthClients.ts so that it was clearer where those important interfaces lived.
  • (If preferred, I can rewrap those two changes above into their own PR)
• AuthClient factory methods can now return a Promise, allowing for lazy loading of sdk libraries. This allows us to use a new library like clerk-react without making it a dependency for every user.
• Addition of an optional hook to AuthClient: useListenForUpdates gives the AuthClient a hook by which to force the AuthProvider to reauthenticate. This is motivated by the fact that the underlying authentication library might change state without user interaction such as through a token timeout, or through SPA user interaction such as a popup window login.
• Updates to the clerk setup template and versioning.

[netlify]

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	538603e
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62d1c947855b0900081d1cad

[Tobbe]

I don't think this PR should not be merged into a 1.0RC branch

Thanks for saying that. Because you're right. Plus I won't have time to give this a proper look until after v1 is out anyway.

[zackdotcomputer]

@Tobbe I'm going to choose to interpret that as v1 will be out very soon :) excited to see it!

[Tobbe]

@Tobbe I'm going to choose to interpret that as v1 will be out very soon :) excited to see it!

You are not wrong... 😃 If you haven't already, you should sign up to our newsletter https://redwoodjs.com/newsletter and you'll get some some exciting news really soon 😉

[jtoar]

@Tobbe I know you've been cranking on some PRs lately, but could I put this one back on your radar too? And @zackdotcomputer no pressure but are you still interested in this one? Love the detailed write up you did in the original post, and now that v1's been out for a few weeks, would love to get this in the next minor!

[zackdotcomputer]

@jtoar happily. I don't think much changed on Redwood side, and the clerk version I was depending on has been released, so it should be a quick update to make this ready to review.

[Tobbe]

I left two random comments, but don't really have time right now to give this one a proper review. I'm really sorry, but looking at my schedule Sunday evening looks to be the earliest I'll be able to get to this. But just to be clear, I'm seeing this in my own Clerk app as well, so really want to see this PR go in.

[Tobbe]

Cool! First time I see one of these out in the wild 🙂 Hadn't even heard about them until the whole React 18 types debacle

[zackdotcomputer]

Ha I copied it from the api/package.json - I also hadn't seen it in the wild yet!

[zackdotcomputer]

I've updated this PR to include @Tobbe's documentation requests and be up to date with the latest Redwood and Clerk versions. However, I'm encountering an issue locally where use of is causing an error saying it is not wrapped by a ClerkProvider despite the fact that you can clearly see in the trace that it is.

I'm going to investigate this further tomorrow to see if I can get to the bottom myself, but if this issue with contexts is something anyone else has seen it would be greatly helpful if you'd let me know what you discovered.

[Tobbe]

For some more details on the externals stuff, @jtoar wrote this

Basically, webpack doesn’t care about it being an optional peerDependency; if it sees require in source code (in this case, dist code cause babel transpiles import to require unfortunately), it’ll try to bundle it. It has to be told another way
If babel wasn’t transpiling import to require, maybe we wouldn’t have to do this. But alas
I also tried including /* webpackIgnore: true */, but babel strips that too! haha

Now on to your comment @zackdotcomputer

Unfortunately, I don't think externs does that for us because it means the compiled code will always use a runtime access and the package will never be bundled in the final product. However, we need Webpack to package the code into the app bundle if it's used since that is the only way it'll be available at runtime.

Hmm, I'm not sure about this.

We don't ever want the clerk libs to be bundled with the RW framework. We do want the clerk libs bundled with the user's app, iff they use clerk as their auth provider. The file Dom updated controls how the RW framework is bundled, not how an enduser app is bundled. (If I understand things correctly)

I'll have a look at your fix Zack, maybe it's exactly the fix we need despite my comments above 🙂

[zackdotcomputer]

@Tobbe thanks and yeah I fully agree with what @jtoar wrote there that you passed along. I think my change above does satisfy your goal.

My understanding is that Redwood's framework ships to enduser having been compiled by Babel but without going through Webpack. The Webpack bundle step, which would decide what packages are ultimately bundled for the enduser's own users, only happens in the enduser's environment when they run a CLI command to build or develop. This is backed up by the fact that searching through the codebase, the only references I can find to the webpack config files are in CLI.

I ran a production build locally and searched through the output dist folders and the only references in there to @clerk/clerk-react that I can find is the one import in the auth clerk client. It does appear that the package is not being irrevocably bundled with the framework.

[Tobbe]

Thanks for working with me on this one @zackdotcomputer! Really appreciate it 🙂

Makes a lot of sense what you're saying. I'll do some digging on my own while CI runs. Unless I find something new I think we can finally get this merged 🕺 💃

[zackdotcomputer]

@Tobbe Ha I was going to say the same to you and @jtoar. This PR has been a bit of a slog, but I appreciate the teamwork on it because working through this has taught me a lot I hadn't yet learned about both RW's internals and Webpack's.

Also excited to get it in and be done with it, though! 😅

[Tobbe]

I'm writing codemods for this PR over here: #5969

[dac09]

@jtoar - just a headsup that we've found that this PR can cause issues on existing projects.

It's not actually breaking - but prints errors that seem scary. Just marking so we don't accidentally release before discussion.

[zackdotcomputer]

Ah 😞 sorry @dac09

What are folks seeing? I tried spinning this up locally over an existing project and didn't see something, which is why I declared it safe to be non-breaking.

[dac09]

Ah 😞 sorry @dac09

What are folks seeing? I tried spinning this up locally over an existing project and didn't see something, which is why I declared it safe to be non-breaking.

Don't worry at all @zackdotcomputer - we're grateful for your contribution here, and it does move things in the right direction - just doing my due diligence here.

It appears like this, both in our framework tests and in tests in a RW proejct:

   console.error
      Warning: An update to AuthProvider inside a test was not wrapped in act(...).

      When testing, code that causes React state updates should be wrapped into act(...):

      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */

      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act
          at AuthProvider (/Users/dac09/Code/redwood/packages/auth/src/AuthProvider.tsx:149:38)

The problem is that the "error" appears in the tests, but doesn't actually fail it. Its merely highlighting an untested state that we weren't catching before due to the async nature of the component - but it does look quite scary and appears for all the component tests that get generated by default.

Tobbe and I looked at the problem, and will look to solve it here: #6058 - it will however be a "breaking" change technically - since existing projects with the default tests will show the error.

[zackdotcomputer]

Ok gotcha - makes sense.