Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(remix-react): add reset function to fetchers #3551

Closed
wants to merge 2 commits into from

Conversation

Vlemert
Copy link

@Vlemert Vlemert commented Jun 23, 2022

I probably shouldn't reopen this discussion with a PR, but it was a great opportunity to gain some insight into how Remix works and even if not landed it might serve some purpose for future discussion around this topic.

Why this change

I think there's a good case to be made for resetting fetchers, and found a discussion on discord that seemed to come to the same conclusion.

The case I ran into is that I have a typeahead that loads data from the server using a fetcher. Every time you click on a search result it gets added to a list and the typeahead closes and clears. When clicking on the typeahead again a new fetch happens to repopulate the typeahead based on the empty query.

If in this scenario we do nothing special, stale data will be shown. This is because fetcher.data will keep the stale value around while loading new data. This is great in a lot of cases, but sometimes you explicitly don't want that to happen. In this case I know the data is stale the moment the user clicks on one of the typeahead suggestions, and I really don't want to show it anymore.

There are probably many other scenarios where fetcher.reset() would be useful, the discussion on discord is about a file upload that the user can clear for example.

There are some solutions to work around this problem:

  1. Keep track of a copy of the fetcher state, clear that whenever I want
let typeaheadFetcher = useFetcher();
let [data, setData] = useState(null);
useEffect(() => {
  if (uploader.state === "done") {
    setData(typeaheadFetcher.data);
  }
}, [typeaheadFetcher])
...
function onItemClick() {
  setData(null);
  // do other stuff
}
  1. Put the typeahead results and the fetcher in a separate component and remount it whenever it needs clearing
function TypeaheadResults({query}) {
  let typeaheadFetcher = useFetcher();
  useEffect(() => {
    // call typeaheadFetcher.load with the current query
  }, [query]);
  
  // render the results from typeaheadFetcher
}

function Typeahead() {
  const [resultsKey, setResultsKey] = useState(0);

  function onItemClick() {
    setResultsKey(resultsKey+1);
    // do other stuff
  }

  return <TypeaheadResults key={resultsKey} />
}

I was going to type a lot of reasons why I believe both these options are bad but Ryan already summarised this perfectly on Discord:
image
(if that is still the current thinking, but I for one believe forcing developers to use useEffect in app code is the opposite of what Remix could be doing)

Why not this change

I have no idea if there are any downsides to having this functionality. There's some mentions in the discord discussion on why it isn't there yet, but I don't see any major reasons why this shouldn't be supported ever.

Formalities

Closes: #2749

  • Docs
  • Tests

Testing Strategy:

yarn test

These tests covers this code:

  • integration/fetcher-test.ts
  • packages/remix-react/__tests__/transition-test.tsx

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 23, 2022

Hi @Vlemert,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 23, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@JeffBeltran
Copy link
Contributor

This makes me happy, I thought I was just doing things weird but the modal form submission does seem to be a good example of where this would be useful vs maintaining local state. Plus the un-mounting trick (while works) messes up my pretty animations lol

Thanks @Vlemert for putting this together

@ryanflorence
Copy link
Member

function onItemClick() {
  setResultsKey(resultsKey+1);
  // do other stuff
}

Won't that set some state that you can use to avoid rendering the previous results when they start typing again?

@JeffBeltran
Copy link
Contributor

I can't speak for the example that @Vlemert left but for me i keep running into this on a project where we are using modals and form submissions.

If i submit a form via a fetcher and get a validation issue and then close the modal and open it back again i see the same validation issue from before, as a user i would expect that if i close modal via a "cancel" action that it would reset my form.

I'll try to come up with a repo to demo my use case before i head out of town this weekend, but for now I just copy the fetcher.data value via useState and treat that as source of truth vs the fetcher.data and then just clear that when needed. Just seems like an extra step.

@MichaelDeBoey MichaelDeBoey changed the title feat(remix-react): Add function to reset a fetcher feat(remix-react): add reset function to fetchers Jul 20, 2022
@justinwaite
Copy link
Contributor

justinwaite commented Jul 31, 2022

I've definitely felt the pain of not being able to reset fetchers. For me it's mostly in modals/slide-outs where I not do not unmount/remount for animation and performance purposes. +1 from me

@ryanflorence
Copy link
Member

Hey folks, can some of you provide working, sample UI on https://remix.new with your use cases?

I'm still not against having fetcher.reset() but every time I sketch out what I think you are trying to do (type aheads with saved selections, modals, animations) I can't quite understand why you need to reset the fetcher.

It seems there's always some piece of application state that's triggering the fetcher (at least in my sketched out use cases) and risks getting your UI out of sync with the fetcher if you're able to "reset" the fetcher.

For example, if you reset the fetcher but don't clear the typeahead input: the input has text in it but the results are now gone. Instead you could just control the input and clear the text and the fetcher will respond (depending on how the app works).

I'm rambling a bit now, but my concern is reset risks your UI getting into unexpected states. Would love some running examples 🙏

@JeffBeltran
Copy link
Contributor

@ryanflorence yup i can work on this, it's on my list anyways so i'll see if i can get that up today as i should have a bit of downtime.

As an aside thank you for coming back to this, legit the only real pain point i have had using remix so far

@wKovacs64
Copy link
Contributor

This may be too contrived and over-simplified, but: https://stackblitz.com/edit/node-4bw59u?file=app%2Froutes%2Findex.tsx

@JeffBeltran
Copy link
Contributor

@ryanflorence I know the UX could be debated in this example but it is contrived after all...

https://stackblitz.com/edit/node-8xvzeb

Really what I want is the ability to reset state that gets derived from the fetcher data. Right now, I need to make a copy of the fetcher data via useState and then manage it from there. This seems a bit boilerplate and makes things hard to read.

@sergiodxa
Copy link
Member

@ryanflorence I know the UX could be debated in this example but it is contrived after all...

https://stackblitz.com/edit/node-8xvzeb

Really what I want is the ability to reset state that gets derived from the fetcher data. Right now, I need to make a copy of the fetcher data via useState and then manage it from there. This seems a bit boilerplate and makes things hard to read.

In this example, if you moved the dialog to another component, you could put the fetcher there, then when the dialog unmount (you close it) and re-mounts (you open it again) it will be a new fetcher with empty data.

@JeffBeltran
Copy link
Contributor

@ryanflorence I know the UX could be debated in this example but it is contrived after all...
https://stackblitz.com/edit/node-8xvzeb
Really what I want is the ability to reset state that gets derived from the fetcher data. Right now, I need to make a copy of the fetcher data via useState and then manage it from there. This seems a bit boilerplate and makes things hard to read.

In this example, if you moved the dialog to another component, you could put the fetcher there, then when the dialog unmount (you close it) and re-mounts (you open it again) it will be a new fetcher with empty data.

funny you say that, i recall doing that in another part of that app... I'm about to head out of town but when i get back next week i'll dig around and see if this was good enough. i feel like there was an edge case to this though...just can't recall and might be more me being new to the framework when i first ran into this issue.

I'd say for now until we get a solid example, this feature might not be needed and is just a by product of how things were done pre-remix.

@thosmos
Copy link

thosmos commented Jan 20, 2023

rant warning

@ryanflorence I just read the discord conversation linked above and the code solution you offered is to add a useEffect and a useState, and then you said, you can resubmit, and I can't think of a product use case where that's not enough. In the first case you're adding unnecessary useEffect and state, and in the second case you're requiring an unnecessary server round-trip (spinner), but then you say you want the opposite, We have two goals with remix, destroy all spinners and useEffect calls. Which is it?

Forms are user driven, and fetcher.Form is usually a call/response thing driven by the user. Sometimes the user wants to start over with a form without another submit to the server (which also requires the server to provide a reset code path, which is strange to expect for a client-only concern). Someone on Discord recently said they have an endpoint just for returning null to work around this flaw. Another person said they put the fetcher in a sub-component and unmount and remount it to get a new one. That's fine if that's where you need it, but bizarre if not.

fetcher.data is a nice way to hang simple reactive UI directly onto the response from the last user action. But when the user wants to reset, and clear away the past, then making the programmer need to jump through extra hoops just to get back to where we started is strange, and honestly annoying. We already declared what the default state of the form is, so why can't we get back to that like a good old form.reset()? I think the use case is already proven by form.reset() and it deserves proof that it's a bad idea to NOT do it.

Ryan you asked for examples, here's a minimal version of the form I made earlier today:
https://stackblitz.com/edit/node-9ta3ig?file=app/routes/index.tsx
If you send a random "email", it changes to accept a verification code, which will always fail, but then there's the reset button to start over with a new email, which doesn't work because the UI is still derived from the previous response even after the form reset. it would be nice if a fetcher.Form's reset input worked automatically to return fetcher.data to its initial value.

I tried doing some variations like the examples above, having the fetcher in a sub component, or layering extra useEffect and useStates on top, but it's all just way more complexity than is needed. Please just allow the reset.

@MichaelDeBoey
Copy link
Member

Hi @Vlemert!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023
@github-actions
Copy link
Contributor

This PR has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from PRs that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed needs-response We need a response from the original author about this issue/PR renderer:react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants