I think we should move all Providers into a provider or state directory instead of them living mixed in with our components.

Actually I would call it contexts. And I might rename these files to AuthContext, DashboardContext, etc.

I see Provider being used more broadly in community but I'm ok with Context too. About where to put them, I think a context directory is good!

As long as we establish a consistent pattern, I'm not too picky!

Totally

NIT: I would probably just name this AuthContext and createContext would take a AuthContextValue type

But there is a difference. The AuthContext is the one created from React.createContext, and the Provider is the wrapper around it that define the values like data and additional functions

I get the difference - I would name it AuthContext for brevity, and also because it's named after the thing it's created by: a hook called createContext. You see the React docs using this pattern here. The interface you defined describes the values available on the context (you're right tho - they are enabled through a provider). That being said, if you prefer the longer name, I'm down. We should just establish a pattern since we'll be using React state more frequently.

Ahh I see. But in this case the Context is being used directly. We can think like Material UI that has a wrapper around the their theme context and named it ThemeProvider as example

Why would this type be potentially undefined? Because the machine hasn't initialized yet? If so, could we do something like:

const AuthProviderContext = createContext<AuthProviderContextValue>({
  authService: undefined
})

Or whatever the un-initialized value for authService would be? This way we can be a little more explicit about what the machine is returning early-state, and we also set ourselves up for the future where the context is returning other methods.

It is ok it uses undefined, it should not happen tho but the useAuth hook is doing the type validation already. I see creating empty context values as an anti pattern. There is a good blog post about this somewhere, I will try to find it.

Please do - I wasn't aware and would love to read more. I'm curious how this pattern handles:

1. data that comes back asynchronously
2. data that errors differently

For example, if you had an auth context that returned the following values:

type AuthContextValue = {
  error: undefined | ApiError
  anotherSpecificError: undefined | ApiError
  currentUser: null | AuthContextQueryUser
  currentOrg: null | AuthContextQueryOrg
  specificUserPermission: null  | AuthContextQueryPerm
}

It might be nice to not have the entire context return value undefined if currentUser and currentOrg have returned but we are still waiting on specificUserPermission. Similarly, we can specify separate errors with this type of pattern, if we need that distinction within the page.

Yes, at somepoint this provider is going to pass values like these instead of machine actors but I thought it would be too much to do right now.

The undefined is just a type thing tho since the initialization happens when the component is used on AuthContext.Provider and pass the value props. It also help us to throw an erro if the user is using the context or the hook outside of the Provider.

About errors I don't think we need to be too much specific. We make that in the machines but I don't see too much vale tho and it adds significantly work.

About the naming, I think I only was able to understand it now 🤦‍♂️ AuthContext is definitely better than AuthProviderContext for sure.

What happens if one of these is undefined because of an error?

If they are undefined, we are going to show the loading state. About the error, good catch, I forgot to handle it.

Not sure if this can apply here, but I fixed a bug recently where the value had been fetched but was falsy so a check like this left us in the loading state forever. Just something to keep in mind, whether falsiness really indicates loading. I like to put "loading" tags on loading states in the machines.

Interesting case, but in theory, any of these data should be undefined or even false.

Buuuut, this should not be here. This is going to be refactored for something like this: https://codercom.slack.com/archives/CJURPL8DN/p1674072426535449?thread_ts=1674071396.499819&cid=CJURPL8DN

I would break these two outcomes out into separate files: one for the case where audit logs should be hidden, and one for their visibility.

What benefits do you see on that?

Great question! A couple:

1. less scrolling through long files
2. easier to read conditionals, e.g:

<ChooseOne>
    <Cond condition={bool}>
        <OptionAComponent />
    </Cond>
   <Cond condition={bool}>
        <OptionBComponent />
   </Cond>
</ChooseOne>

I know we have a lot of longer files in our repo but this just isn't a pattern I've seen anywhere else. Many companies turn on lint rules to limit file length.

Thx, it definitely makes easier to read conditionals. About long files I personally like them haha it sounds a tooling thing since you can collapse the root functions in the editor tho. What I usually think about it is, if the component or code is only used in that file, it should be on the file so it is easier to find, modify, delete and I know it only affects that scope. When it has your own file, I assume it has been used in more than one place so I have to be more carefully.

But no strong opinion, I know a lot of people don't like it 😄

Sure thing!

Personally I also like shorter files because I feel less likely to overlook something. I think the concern about stuff that's reused is important but that we can put everything related in one folder, like Kira says. I won't be heartbroken either way though.

Lets go with shorter files!

Another thought, is it ok just to move the Paywall? I would avoid to have one more level of prop drilling

Ya, definitely!

What do you think about keeping this context 'pure' meaning it doesn't really dictate UI at all? Then we could expose isLoading and isError and the consumer could handle those UI changes. Benefit would be keeping our UI all in one place so folks opening up that page component can see how it's handled at first glance, without having to dig into the context.

Not sure if I got it. I don't see too much value on having it pure since the value, from what I see, is to have a provider that handle that in one single place but maybe I'm missing something. I based this provider on https://kentcdodds.com/blog/authentication-in-react-applications

I think in that example, he's showcasing functionality instead of organization, but I'm happy to talk it through in Discord. It's also not a blocker.

Interesting. I'm usually agnostic about where to put these, but I don't know if I've ever seen a provider render a spinner. But I see that Kent really does it intentionally:

  // 🚨 this is the important bit.
  // Normally your provider components render the context provider with a value.
  // But we post-pone rendering any of the children until after we've determined
  // whether or not we have a user token and if we do, then we render a spinner
  // while we go retrieve that user's information.```

Hmm, I could see that. I was looking at his comment below:

The key idea that drastically simplifies authentication in your app is this:

The component which has the user data prevents the rest of the app from being rendered until the user data is retrieved or it's determined that there is no logged-in user

So I figured the component is the consumer of the context, and it should have something like this:

const {isLoading} = useAuth()

if {isLoading} return <Loader/>

return (
 <App />
)

I guess this is an argument for colocation, kind of similar to what we were discussing with long files. But just because it isn't a pattern I've seen before doesn't mean it's wrong! I'm happy to keep the loaders in the context.

But just because it isn't a pattern I've seen before doesn't mean it's wrong!

I agree 100%. In this case, I just think would be too much to have another component to handle the spinner. If at some point it starts to do more and more stuff, I think would make sense to split it.