Didn't see this function get used anywhere, but also, one concern I have with the function is that even though it groups the mutable/immutable params, they're still all in one array, so you have to iterate through it to see where one group starts, and the other stops

I feel like, if this does need to get added back down the line, I'd consider these changes:

1. Returning this as an object, so that you know which one is which from a glance

   type Return = {
     immutable: TemplateVersionParameter[];
     mutable: TemplateVersionParameter[];
   }

2. Maybe use some custom type predicates under the hood, so that TypeScript is able to say that the mutable property is always true inside the mutable array, and always false inside the immutable array

The only thing that changes with this chunk of code is that a React fragment got added, so I could squeeze in the DuplicateWarningMessage component. Everything else is the same – it just changed by an indentation level.

you could also put it next to the {Boolean(error) ... and that might look nice

you could also put it next to the {Boolean(error) ... and that might look nice

this is clever, but maybe too clever. 😅 it took me a second to realize what this does, and others who are less familiar with useReducer could probably be quite confused by this. can we just use useState instead?

Yeah, that's fair, and I figured I'd get some pushback on that.

I use the action-less reducer pattern a lot in my personal code, where I want to make my expected state transitions super, super explicit (in this case, making it clear that state can only ever transition from false to true, and shouldn't be undone). It helps that useReducer has function overloads to augment the type of the dispatch if it is action-less

But as far as readability/communication, it's probably a bit much/too convoluted for a simple use case like this. I'll change this

I feel like this should be an "info". Nothing actually went wrong, the user isn't doing anything sketchy. It's good context to provide, but I think warnings should result from users doing something worth complaining about, not from following the "blessed path"

Yeah, agreed. Will get that fixed up

oh I really don't like setting this trend 💀 I don't exactly know what to recommend to fix it tho, maybe @BrunoQuaresma does?

Yeah, I'm not a fan, either, but I wasn't sure about other options.

One alternative is using the base MemoryRouter component (which does support children), but my main worries are:

• Then the test isn't using the the main setup function we've got defined for most other cases
• I kind of get the sense that the base MemoryRouter component might become deprecated down the line, because createMemoryRouter is more in line with their new data loading patterns. (It's also the React Router team, so they're not afraid to ship major breaking changes).

I'll see if I can find any discussions for testing hooks with the new APIs, but if anyone can come up with an improvement, I would gladly remove this in a heartbeat

The renderWithAuth function should let you pass children routes. Can you explain a bit more what issues did you face on tests?

So, the main issue is that I'm stuck testing an individual hook, rather than a whole component. Ideally, my function (however it's implemented) would give me back the custom hook result, as well as a React Router router object (so I can check its internal state after modifying the hook result).

But what I was running into was that I couldn't figure out how to set things up so that I could get both values back. It seemed like it was one or the other, and I chose the one that would've been harder to mock out. I can't remember the precise syntax issues, but I'll try to recreate the problem after the FE Variety in a few minutes

I guess mocking out useNavigate itself is an option, but I feel like that makes the tests get further from our real code

We use https://react-hooks-testing-library.com/, not sure if that is helpful but if that is the only way, I'm good with that for now.

Yeah, I didn't realize that the React Hooks test library was a separate initiative, but renderHook comes from there, so sadly, there's nothing in the API docs that I haven't tried yet

I went through things again, and my take is that we're stuck choosing from two less-than-ideal compromises:

1. We render with the current approach, which is kind of cursed, but it side-steps some of the limitations of hooking renderHook up to a React Router MemoryRouter
2. We go with renderHook and swap createMemoryRouter for the direct <MemoryRouter> component, but then we have to mock out parts of React Router itself (like useNavigate), because this approach prevents us from accessing the router object itself

Writing this out, so that we at least have something to reference in the future:

So, the main problem stems from the fact that while renderHook lets you test custom hooks and supports some form of dependency injection via its wrapper property, the API assumes that all components being injected support direct children composition. React Router's createMemoryRouter, on the other hand, assumes that you have all components defined ahead of time, so it doesn't support children in the same way, nor does it give you the ability to add more routes after the router has been created.

If I were only testing the React Query parts, I could do this:

const testResult = renderHook(() => useWorkspaceDuplication(MockWorkspace), {
  wrapper: ({ children }) => (
    <QueryProvider client={testQueryClient}>
      {children}
    </QueryProvider>
  )
}); 

But since the code also tests React Router, there's two choices:

1. Use the <MemoryRouter> component directly

const testResult = renderHook(() => useWorkspaceDuplication(MockWorkspace), {
  wrapper: ({ children }) => (
    <QueryProvider client={testQueryClient}>
      <MemoryRouter>
        <Routes>
          <Route path="/" element={<>{children}</>} />
          <Route path="/templates/:template/workspace" element={<CreateWorkspacePage />} />
        </Routes>
      </MemoryRouter>
    </QueryProvider>
  )
}); 

This seemed to hold up until I needed to verify that the navigation went through correctly. There wasn't really a way to access the router and verify the navigation had happened, and basically all the older StackOverflow answers said that you would need to mock out useNavigate itself

2. Use createMemoryRouter to make a memoryRouter to feed into <RouterProvider>
   This is where things get wonky – it basically turns into a chicken-or-the-egg situation, mainly because RouterProvider does not accept children, and memoryRouter is locked tight after being created.

   I need a component that accepts children in some way, so I can hook it up to renderHook's wrapper. But createMemoryRouter can't make a memoryRouter object to hook into a provider unless it has all the page views ready to go in advance (including renderHook's mock component).

So, writing this out gave me a slightly more cursed idea, but it might work better with all the existing APIs. I'm going to explore it more tomorrow

Would need to test this out (and make sure it's resilient to re-renders), but uh...it sure is something:

function renderHookWithRouter(workspace?: Workspace) {
  const queryClient = new QueryClient({
    defaultOptions: {
      queries: {
        retry: false,
        cacheTime: 0,
        refetchOnWindowFocus: false,
        networkMode: "offlineFirst",
      },
    },
  });

  // Easy to miss – there's an evil definite assignment via the !
  let escapedRouter!: ReturnType<typeof createMemoryRouter>;

  // eslint-disable-next-line testing-library/render-result-naming-convention -- Cursed stuff to make sure both router and hook result are exposed
  const renderHookResult = renderHook(
    () => useWorkspaceDuplication(workspace),
    {
      wrapper: ({ children }) => {
        // eslint-disable-next-line react-hooks/rules-of-hooks -- This is actually processed as a component; React just isn't aware of that
        const [readonlyStatefulRouter] = useState(() => {
          return createMemoryRouter([
            { path: "/", element: <>{children}</> },
            {
              path: "/templates/:template/workspace",
              element: <CreateWorkspacePage />,
            },
          ]);
        });

        escapedRouter = readonlyStatefulRouter;

        return (
          <QueryClientProvider client={queryClient}>
            <RouterProvider router={readonlyStatefulRouter} />
          </QueryClientProvider>
        );
      },
    },
  );

  return {
    ...renderHookResult,
    router: escapedRouter,
  };
}

Guess the question is: as horrific as this is, are the result and API stable enough that we can abstract the nasty implementation details behind a function, and pretend they don't exist? Is this worth it?

Update: this is cursed and evil, but it works, and I like how it simplifies the testing, making sure the wacky mocking stays in one spot

I'm going to clean this up, make sure it supports any kind of hook, and post this in the dev channel to get people's thoughts