meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
571 stars 157 forks source link

useTracker subscription not cleaned up when deps change. #351

Closed sih99 closed 1 year ago

sih99 commented 2 years ago
const useSubReady = (pagination) =>
  useTracker(() => {
    const { filters, page, perPage, sort } = pagination;
    const subPagination = Meteor.subscribe("pagination.members", filters, page, perPage, sort);

    return {
      loading: !subPagination.ready(),
    };
  }, [pagination]);
  const [pagination, setPagination] = useState({
    page: 0,
    perPage: 15,
    filters: {},
    sort: { createdAt: -1 },
  });

 const { loading: dataLoading } = useSubReady(pagination);

 const onClickButton = () => {
  setPagination((prev)=> ({...prev, page:prev.page + 1}))
}

It's happening with old subscriptions do not cleaned up and remain when pagination changes. the picture below shows the subscriptions in browser when pagination changes. 스크린샷 2022-01-27 오후 5 44 40

how can i solve this problem?

CaptainN commented 2 years ago

Is this still a problem if you remove deps?

const useSubReady = (pagination) =>
  useTracker(() => {
    const { filters, page, perPage, sort } = pagination;
    const subPagination = Meteor.subscribe("pagination.members", filters, page, perPage, sort);

    return {
      loading: !subPagination.ready(),
    };
  });
sih99 commented 2 years ago

thanks for answering @CaptainN

if remove deps, no problem. but I cannot get a new subscription about changing the pagination.

the subscription will stay in params [0,0,15,{createdAt: -1}]

CaptainN commented 2 years ago

I think maybe you used an empty array, instead of removing the array? If you remove the array, the useTracker should respond to your variable changes. If you use an empty array, it will lock the variables.

timsun28 commented 2 years ago

I'm having the same issue as described here and I can't seem to figure out what I'm doing wrong here: In my code I have a text search field that alters the subscription so I can perform a text search on the server side.

const [textSearch, setTextSearch] = useState("");
const documentsLoading = useTracker(() => {
    // Note that this subscription will get cleaned up
    // when your component is unmounted or deps change.
    const handle = Meteor.subscribe("documents", textSearch);
    return !handle.ready();
}, [textSearch]);

If I set the state initially to something, the search filter works properly, but if I set it to '' and use the setTextSearch function to change it, it will keep the original "" subscription alive and not update the local database correctly.

Do you have any suggestions on how to properly implement this?

meteor-publication-issue

CaptainN commented 2 years ago

I'm not sure what the heuristic is behind Subscriptions, but I do know there is some "magic" there. It does a bunch of things to reuse existing subscriptions even after a computation that previously set up a subscription has been cleaned up. I'm not super clear on how that works, but I bet this has something to do with that.

What you should find though is that the computations are indeed stopped. If they are not, that's definitely a bug.

BTW, I wonder if you have the same problem if you simply omit the deps array?

const [textSearch, setTextSearch] = useState("");
const documentsLoading = useTracker(() => {
    // Note that this subscription will get cleaned up
    // when your component is unmounted or deps change.
    const handle = Meteor.subscribe("documents", textSearch);
    return !handle.ready();
});  // Don't use an empty array, leave it out.
timsun28 commented 2 years ago

I've removed the dependency array and this seemed to have fixed my issue. I'm happy I've got it working now thanks to your suggestion.

The example on the atmosphere page clearly describes the addition of the parameter as a dependency and how this subscription will get cleaned up when your component is unmounted or deps change. I'm hoping this can be cleared up for others who are trying the same thing as I did.

CaptainN commented 2 years ago

Maybe that wording needs to be cleaned up a bit. It's not guaranteed that subscriptions will be cleaned up. It is only guaranteed that computations will be cleaned up. There is further magic that Meteor uses to determine whether subs get cleaned up, that I haven't fully dived in to.

CaptainN commented 2 years ago

BUT - I asked if there was a difference, in part because it should behave the same, with and without deps. This may merit some further investigation.

timsun28 commented 2 years ago

Hi, Yes there is a difference with and without deps. I use the meteor chrome devtools to keep an eye on the subscriptions and collection size and when I tried to subscribe with the deps it would create a new subscription each time the deps change.

Because this happened everytime the search box updated it would end up with 5 subscriptions if you were to search for "test". And because the initial empty search subscription was still active, the local collection was never cleared with the documents that didn't match on the server side.

After removing the deps, I could see that the original subscription was updated with the new params. And so the minimongo was correctly updated to only show the matching documents.

If it would help you, I can create a quick demo on github with the two options and how they differ from each other.

CaptainN commented 2 years ago

That could potentially be very helpful. Maybe we can even turn that in to some tests. Thanks for any contributions!

timsun28 commented 2 years ago

I've replicated the issue in this typescript example project.

I have removed the autopublish package and created a publication for the links collection with the search parameter. In the info.tsx file are two subscriptions (one commented out). Where the uncommented version is giving the desired result and where the second option is not giving the correct solution by not cleaning up the old subscription.

If I could help to turn this into some tests or help you with anything else, let me know!

dotmlj commented 1 year ago

Hi, any progress on this? 😅

We recently updated our app to the latest Meteor 2.7.3 and saw a huge decrease in performance because of this bug. We have implemented a workaround, but we would love to be able to use useTracker with dependency again.

Thanks 🙌

Grubba27 commented 1 year ago

Hey @dotmlj, what was the implemented workaround? Was it the one that @CaptainN mentioned in this thread?

dotmlj commented 1 year ago

@Grubba27 sorry, I should have included that. We ended up not using any deps and moving the most affected subscription to a parent component. Performance is still not as it use to be, so we are eager to get a real fix in place..

Would it be possible to simply use an older version of react-meteor-data that we know worked with useTracker deps with the latest Meteor version (2.7.3), like react-meteor-data 2.4.0 that we where using with Meteor 2.7.1, or are the latest versions tied close together?

Grubba27 commented 1 year ago

From my point of view, I do not think they are tied together and I've checked the package.js file as well. Things should just work out, I guess. I do not have an app here for testing this

CaptainN commented 1 year ago

There was an attempted fix for this last May - can anyone confirm that it didn't fix this?

azambranogalbis commented 1 year ago

Hi! I just faced this issue, both with react-meteor-data versions 2.5.1 and 2.6.0. I can also confirm that workaround proposed at https://github.com/meteor/react-packages/issues/351#issuecomment-1132290180 makes it work as expected.

Grubba27 commented 1 year ago

It was published under the namespace react-meteor-data@2.6.2. If this issue is still not solved, please reopen it.