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

Easy way to register to for a one time promise callback? #58

Closed
eyalw opened this issue Feb 15, 2016 · 23 comments
Closed

Easy way to register to for a one time promise callback? #58

eyalw opened this issue Feb 15, 2016 · 23 comments

Comments

@eyalw
Copy link
Contributor

eyalw commented Feb 15, 2016

Rendering different promise statuses (pending, setteled, etc) is easy (yay!).
Performing a function one-time once a promise settles seems hard.
It's very common that I want to perform some logic when a promise is settled - for example:

  • forms (load data into state): fetch form data, once available, setState({ data: value }).
  • forms (post data, then do something): when submitting a form - on success, redirect to so other page (with react-router), or then - show success toaster.

if i do this:

  // no good for me
  componentWillReceiveProps(nextProps) {
    nextProps.fetchAccountPs.then(account => this.setState({ account }));
  }

It gets called every time props change, even though the fetch promise does not change, not good. I only want it to get called once. when (ps.pending => ps.setteled).
if the promise mapping changes and fetch occurs again, only then call the .then() again.

right now I need to keep track of when (ps.pending => ps.setteled) happens myself which is tedious.
would be really nice to be able to do

connect({
  fetchAccountPs: {
     method: "GET",
     url: "/some/url",
     callback: "onAccountFetched"    // <==  suggestion
  }
)...

and the HOC would call my component exposed onAccountFetched(value) once, using a ref.

what do you think?
is there another existing way i'm missing?

@ryanbrainard
Copy link
Contributor

Have you tried using andThen as @hnordt suggested in #47?

@eyalw
Copy link
Contributor Author

eyalw commented Feb 17, 2016

@ryanbrainard, can you explain how it helps in the scenario I described?
I see @hnordt has a prop array named 'history' given to the wrapped component,
and promise value is pushed into that array.

I actually want to put the promise value into the wrapped component state (for example).
or - redirect (which will require access to this.context.router with react-router API).

@arbesfeld
Copy link

FWIW, I also have had to do this, and have been using a pattern like this:

componentWillReceiveProps(nextProps) {
  const { createProjectResponse } = nextProps;
  if (createProjectResponse && createProjectResponse.fulfilled) {
    this.onDismiss();
  }
}

But it would be nice if the createProject just returned a promise.

@eyalw
Copy link
Contributor Author

eyalw commented Feb 17, 2016

@arbesfeld this works because onDismiss() will close the view component you are in.
but if it didnt, (e.g. setState, or update view) - on the next "willReceiveProps" the onDismiss() will be called again and again... so it doesn't make sense for me.

Really surprised such a classic scenario does not have an answer in RR. I really like the design pattern of RR, its really simple and declarative, no boilerplate code. I wish the HOC will call a callback I defined,
or at least have the createProject() return a promise.. (less prefered)

@eyalw
Copy link
Contributor Author

eyalw commented Feb 20, 2016

@ryanbrainard any idea?

@eyalw
Copy link
Contributor Author

eyalw commented Feb 23, 2016

What I ended up doing was wrapping my component with my own HOC below the Connect HOC, which middlewares the willReceiveProps lifecycle method and checks for a change in promiseState.settled value between this.props and nextProps, and triggers an exposed function on the wrapped element.

I wish it was a built in function though

@ryanbrainard
Copy link
Contributor

@eyalw Sorry for the delay getting back to you. I would like to support your use case, but I'd like to understand it a little better:

Is there a reason you want to pass in the callback as a ref? Does that give any advantage over proving a function that takes the wrapped component as an argument? I don't use refs very much, so perhaps I'm missing something.

@ryanbrainard, can you explain how it helps in the scenario I described? I see @hnordt has a prop array named 'history' given to the wrapped component, and promise value is pushed into that array.

I don't think that history is an array, but rather the history from https://github.com/reactjs/history being injected as a prop by react-router. The push should be doing redirect, not adding to an array.

What if meta contained a reference to the wrapped component with, so you could do something like this?

connect({
  fetchAccountPs: {
     method: "GET",
     url: "/some/url",
     andThen: (_, meta) => {
        meta.wrappedComponent.onAccountFetched()
        return {};
      }
  }
)...

Not sure if this is the best place to expose this, but just spitballing a bit. I'd like to do this in a supported, clean way, but would like to understand the requirements a bit better to see if RR should be calling the function on your behalf, if the promise should be exposed in meta, or what is the best approach. If anyone want to open a PR with idea, that would be welcomed as well. I personally don't have any need for this, so a PR will probably be the faster than waiting for me to do something :)

@eyalw
Copy link
Contributor Author

eyalw commented Feb 23, 2016

@ryanbrainard I like your idea. Your API seems more flexible, and you can control the function being called better, and add conditions and pass different values.

I do also like it when an API offers me to code the common case in shorter way, which I think is: wrappedComponenet[calbbackName](err, returnVal) - so I guess I would have preffered to have a shorthand as well

As to your question about my use case, its mostly stuff like:

onFetchAccountResponse(err, account) {
  if (!err)
    this.setState({ account }); // load data into a state-binded-form-fields
}
onSaveAccountResponse(err) {
  if (!err) {
    showNotification('success', 'Wohoo! Account saved'); // one time GUI notification
    this.context.router.redirect('/somwhere'); // redirect
  }
}

onChangePasswordResponse(err) {
  if (!err) {
    showNotification('success', 'Your password was changed'); // notify again, and:
    this.setState({ formData: null }); // clear the form fields
  }
}

@neezer
Copy link
Contributor

neezer commented Feb 23, 2016

I'll chime in in support an implementation for this behavior, as #66 (duplicate of this issue, effectively) illustrates my use case.

Also, @hnordt's solution doesn't work for me since I'm using react-router v2, where router is defined on this.context (this.props.history is being deprecated).

You can still workaround that fact using the singleton history, but you're going to run into issues with universal apps with differing history strategies (in-memory vs browser).

@eyalw
Copy link
Contributor Author

eyalw commented Feb 24, 2016

@ryanbrainard I'm interested to learn, when andThen() will be called, and will make a call to a function on my component, what is the props of the component at that instant? are they already updated with the new fetched data? was willReceiveProps already called for the wrapped component or not?

@ryanbrainard
Copy link
Contributor

@eyalw andThen is called in the callback to setState. This is inside of the promise fulfillment handler, so it will be called after the fetch successfully returns and the JSON is parsed.

what is the props of the component at that instant?

The prop should be fulfilled

are they already updated with the new fetched data?

yes

was willReceiveProps already called for the wrapped component or not?

yes


I think what you are getting at here is that it might not work since it's already updated. then by contrast is called before setState and could be used with an identity request, but feels like a crappy workaround:

connect({
  fetchAccountPs: {
     method: "GET",
     url: "/some/url",
     then: (value, meta) => {
        meta.wrappedComponent.onAccountFetched()
        return {value, meta};
      }
  }
)...

So, if we create a new place to do this kind of thing, where would be the best place in here so that to happen? ...or, would then or andThen actually work? It would be nice to leverage them if possible so we don't have a sprinkling of hooks everywhere, so something that uses them as is or extends them would be prefered, but not totally against adding something new if its needed.

@eyalw
Copy link
Contributor Author

eyalw commented Feb 24, 2016

having both then and andThen as 2 different times to hook into - before and after the props have passed down to the wrapped component - seems great to me.
I was asking because I was thinking my wrapped component callback might be using "this.props" to get the new value, or must it be passed in the callback.

@yarinm
Copy link

yarinm commented Feb 29, 2016

I would really be happy to see @ryanbrainard suggestion implemented.
But why do we need to return {value, meta}? can't we also support an empty return ?

@amoskl
Copy link

amoskl commented Feb 29, 2016

+1

2 similar comments
@uripo
Copy link

uripo commented Mar 1, 2016

+1

@XAMeLi
Copy link

XAMeLi commented Mar 1, 2016

+1

@gaugau
Copy link

gaugau commented Mar 18, 2016

I encountered the same scenario as @eyalw. Where I just want to trigger an action eg. updateUser and then performing an action (eg. show notification). I think it's common case and the documentation should be updated to show an example of how to achieve it with RR.
Currently, I need to hook into componentWillReceiveProps to check if the response has been fulfilled and perform the callback action. But any changes on props later could trigger the same callback action multiple time. :(

@eyalw
Copy link
Contributor Author

eyalw commented Jul 23, 2016

@ryanbrainard any updates on this issue? I think meta.wrappedComponent is a very good addition to the API.

@ryanbrainard
Copy link
Contributor

@eyalw Thanks for bringing this up again and opening the PR.

@yarinm, the change in #128 should allow for an empty return.

eyalw added a commit to eyalw/react-refetch that referenced this issue Jul 25, 2016
@Soul-Burn
Copy link

Soul-Burn commented Jul 27, 2016

In my case, I want to show a dialog window when the result was fulfilled.
In e.g. material-ui this is done by sending a open="true" prop to the dialog, usually coming from the state/props of the parent. It is then closed when a button is clicked and the handler sets the state of the parent.

While the fetch promise prop doesn't exist, the dialog is not shown.
When it exists and pending, I show a loading animation.
When it fulfilled, I show the dialog.
However, there is now no way to now dismiss this dialog, as the promise will remain until it is replaced/refetched.

A one time callback like proposed here will let me set the dialog state.
I can work around this by defining a "value" fetcher and relying on this value rather than the promise's fetching state.
However, in my case, and along the lines of RR taking care of state, it makes more sense to have a way to dismiss the existing PromiseState and return it to how it was before the fetch function was called to designate the of this flow.

Something like this could work (not tested):

connect(props => ({
  shouldShowDialog: { value: false },
  dismissDialog: () => { value: false},
  postStuff: data => ({
    stuffRequest: {
      method: 'POST',
      url: '/stuff',
      andThen: () => ({
        shouldShowDialog: { value: true }
      })
  })
}))(MyApp)

Then my code could use the value in shouldShowDialog combined with the stuffRequest promise state.

That said, is there anything limiting us from passing this into the function and simply call stuff on it in the andThen?

connect(props => ({
  postStuff: data, component => ({
    stuffRequest: {
      method: 'POST',
      url: '/stuff',
      andThen: () => ({
        component.doStuff();
        return {};
      })
  })
}))(MyApp)

@ryanbrainard
Copy link
Contributor

is there anything limiting us from passing this into the function and simply call stuff on it in the andThen?

I don't believe there is (didn't try it), but @eyalw 's proposal in #139 has the nice property of also working in cases where you aren't calling a fetch function.

ryanbrainard added a commit that referenced this issue Jul 28, 2016
#58 Added wrappedInstance to the meta object
@Soul-Burn
Copy link

Makes sense.

What about the proposal of dismissing a fetch, back to how it was before it was fetched?

So we could have rendering like this example:
no fetch - nothing shown
pending - loading animation
fulfilled - some result, possibly a dialog showing
rejected - another result, possibly a dialog showing
Once the dialog is done, the fetch can be dismissed to move it back to the original state.

@eyalw
Copy link
Contributor Author

eyalw commented Jul 29, 2016

Can we close this issue now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants