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

   typeReturn={immutable:TemplateVersionParameter[];mutable:TemplateVersionParameter[];}

2. Maybe use some custom type predicates under the hood, so that TypeScript is able to say that themutable property is alwaystrue inside themutable array, and alwaysfalse inside theimmutable array

The only thing that changes with this chunk of code is that a React fragment got added, so I could squeeze in theDuplicateWarningMessage 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 withuseReducer could probably be quite confused by this. can we just useuseState 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 baseMemoryRouter 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 baseMemoryRouter component might become deprecated down the line, becausecreateMemoryRouter 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

TherenderWithAuth 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 outuseNavigate itself is an option, but I feel like that makes the tests get further from our real code

We usehttps://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, butrenderHook 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 hookingrenderHook up to a React Router MemoryRouter
2. We go withrenderHook and swapcreateMemoryRouter for the direct<MemoryRouter> component, but then we have to mock out parts of React Router itself (likeuseNavigate), 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 whilerenderHook lets you test custom hooks and supports some form of dependency injection via itswrapper property, the API assumes that all components being injected support directchildren composition. React Router'screateMemoryRouter, on the other hand, assumes that you have all components defined ahead of time, so it doesn't supportchildren 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:

consttestResult=renderHook(()=>useWorkspaceDuplication(MockWorkspace),{wrapper:({ children})=>(<QueryProviderclient={testQueryClient}>{children}</QueryProvider>)});

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

1. Use the<MemoryRouter> component directly

consttestResult=renderHook(()=>useWorkspaceDuplication(MockWorkspace),{wrapper:({ children})=>(<QueryProviderclient={testQueryClient}><MemoryRouter><Routes><Routepath="/"element={<>{children}</>}/><Routepath="/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 outuseNavigate itself

2. UsecreateMemoryRouter to make amemoryRouter to feed into<RouterProvider>
   This is where things get wonky – it basically turns into a chicken-or-the-egg situation, mainly becauseRouterProvider does not acceptchildren, andmemoryRouter is locked tight after being created.

   I need a component that acceptschildren in some way, so I can hook it up torenderHook'swrapper. ButcreateMemoryRouter can't make amemoryRouter object to hook into a provider unless it has all the page views ready to go in advance (includingrenderHook'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:

functionrenderHookWithRouter(workspace?:Workspace){constqueryClient=newQueryClient({defaultOptions:{queries:{retry:false,cacheTime:0,refetchOnWindowFocus:false,networkMode:"offlineFirst",},},});// Easy to miss – there's an evil definite assignment via the !letescapedRouter!:ReturnType<typeofcreateMemoryRouter>;// eslint-disable-next-line testing-library/render-result-naming-convention -- Cursed stuff to make sure both router and hook result are exposedconstrenderHookResult=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 thatconst[readonlyStatefulRouter]=useState(()=>{returncreateMemoryRouter([{path:"/",element:<>{children}</>},{path:"/templates/:template/workspace",element:<CreateWorkspacePage/>,},]);});escapedRouter=readonlyStatefulRouter;return(<QueryClientProviderclient={queryClient}><RouterProviderrouter={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