TanStack / router

🤖 Fully typesafe Router for React (and friends) w/ built-in caching, 1st class search-param APIs, client-side cache integration and isomorphic rendering.
https://tanstack.com/router
MIT License
8.16k stars 637 forks source link

Memory leak in test from 1.18.3 #1263

Closed JoseBuendiaDigio closed 6 months ago

JoseBuendiaDigio commented 8 months ago

Describe the bug

When I try to update @tanstack/react-router to 1.18.4 I'm getting a memory leak, I tried with all next versions, 1.19.0 and 1.19.1 and same error accurs.

This is only ocurring in jest tests

1.18.4 version supposely only affect to ssr but I'm not using SSR

Your Example Website or App

https://stackblitz.com/edit/github-t9bqm9?file=src%2Fmain.tsx

Steps to Reproduce the Bug or Issue

runnig tests

Expected behavior

Not get a memory leak

Screenshots or Videos

image

Platform

Additional context

No response

schiller-manuel commented 8 months ago

so v1.18.3 does not have this error? can you please fill your enviroment information, such as

JoseBuendiaDigio commented 8 months ago

1.18.3 not have this error.

We tried in linux and windows, both recent versions. node 20.11 and 20.10 and pnpm latest version (8.15)

schiller-manuel commented 8 months ago

can you please provide a minimal example repo?

JoseBuendiaDigio commented 8 months ago

I have attempt it but it's difficult due to dependencies.

I will try to explaint what we are doing.

We have a renderByComponent method, image That it's used to put a component inside the router context, when we want to render only a component. We have the memory leak in this method. But only in one test suite, all the others are running as usual.

We have another to render the routes but here we don't have the problem when navigating to the route that it is using this component. image

And the component it's quite simple, have a useMatchRoute(), useTranslation(), useStyles() useQuery(), and a view that isn't to much complicated This is the only component that it is using useMatchRoute() I have tried to remove it to see if the problem dissapears but no. Same with RQ, I tried to remove it but same problem.

JoseBuendiaDigio commented 8 months ago

Ok, I found something, when I comment the <Outlet /> that the component have it's working

schiller-manuel commented 8 months ago

can you put this into a small reproducer?

JoseBuendiaDigio commented 8 months ago

Should be something like the next one but I can't reproduce it by the moment https://stackblitz.com/edit/github-t9bqm9?file=src%2Fmain.tsx

schiller-manuel commented 8 months ago

any luck with the reproducer?

JoseBuendiaDigio commented 8 months ago

Seems that yes, if you try with 1.18.4 isn't working but with 1.18.3 is working

JoseBuendiaDigio commented 7 months ago

Any news about this?

SeanCassiere commented 7 months ago

@JoseBuendiaDigio, we are still waiting on a reproducing example.

Without a working reproducer with TSR in your workflow, we can't diagnose what exactly about your setup doesn't play well with TSR.

Also, this is the working version of the stackblitz link that you provided above.

JoseBuendiaDigio commented 7 months ago

Problem comes from using Outlet in notFoundComponent in 1.18.3 works but in 1.18.4 not, this is done for test purpose as talked here https://github.com/TanStack/router/discussions/583

SeanCassiere commented 7 months ago

Problem comes from using Outlet in notFoundComponent in 1.18.3 works but in 1.18.4 not, this is done for test purpose as talked here #583

Why are you trying to render an Outlet within the notFoundComponent?

The notFoundComponent shouldn't ever be rendering an <Outlet>.

This may have just been a misconfiguration, but the notFoundComponent is a standalone component that is rendered at a Route either when the route you are navigating to doesn't exist or if you trigger the component by throw notFound(...).

Edit: I re-read the discussion you mentioned, specifically this comment, and this is not a supported use-case. The <Outlet> component is used to render the matches at that point of call, but when you try to render it in the notFoundComponent, it, of course, wouldn't have any matches since you are already at the 'not-found' state. From there on, all it takes is a component that's not stable/un-memoed and re-renders, or even just React's StrictMode, and its not too hard to imagine why you'd have a memory leak.

JoseBuendiaDigio commented 7 months ago

And how can we solve the discussion problematic? This should have an official way to do it. no?

SeanCassiere commented 7 months ago

And how can we solve the discussion problematic? This should have an official way to do it. no?

Since this is all for testing components in the Router, maybe you could: Use the memory history adapter and create an instance of the Router with just a single Route that only renders the component you want to test.

We don't have a guide for testing with TSR since this depends on your setup and how you've got things wired up.

IMO: I honestly don't use unit tests for frontend components and rather use integration tests like with Cypress since it's the actual ui that being rendered that matters.