feat: add ability to provide function in identity requests (fixes #177)

[ipanasenko]

This PR fixes #177

It's now possible to pass a function to identity request's value prop:

const fetchUser = id => fetch(`/users/${id}`);

connect(props => ({
  user: {
    value: () => fetchUser(props.match.params.id),
    comparison: props.match.params.id
  }
}))(Profile);

[ryanbrainard]

Thanks for working on this! Looks good at first glance, but will look into the tests and corner cases a little more later.

Since someone could have theoretically provided a function before, I guess this should be considered a breaking change..

Oh, can you update the API doc and readme for this change too?

[ipanasenko]

@ryanbrainard updated docs

[slorber]

@ipanasenko @ryanbrainard can we publish this as beta on npm? I'd be happy to test that implementation on a real project

[ipanasenko]

@slorber I was very lazy, so I just built dist and copied it into my project's node_modules. You can fetch my branch and do the same ;)

[ipanasenko]

@slorber or modify your package.json to look like this: "react-refetch": "ipanasenko/react-refetch#feat/identity-function-dist",

[ryanbrainard]

@slorber @ipanasenko I just published this as a beta release: react-refetch@2.0.0-0

[slorber]

thanks,

i've just tested and it seems to work fine

[ryanbrainard]

I just realized a small wrinkle in all of this. I think it can be solved just by documentation, but here it is:

If an app does not implement comparison, the default implementation compares value, so that means that if the value function is created inside of the connection function, it will be re-created every time and be considered a change. For example, in the documented example:

connect(props => ({
  usersFetch: {
    value: () => someAPI.list('/users')
  }
}))(Users)

someAPI.list('/users') is going to get called every time because a new function is created every time.

Compare that to:

const valueFunc = () => someAPI.list('/users')
connect(props => ({
  usersFetch: {
    value: valueFunc
  }
}))(Users)

Here valueFunc is created externally, so the function itself doesn't change. In fact, because comparison is not implemented it won't ever be called again.

As long as both comparison and value are set when value is a function, it should work as expected, but I can see this biting someone because it's not very obvious. At a minimum, we should make that clearer in the docs, but also thinking if we should enfore that as a rule or at least a warning. Thoughts?

Also, in the example above, it would probably be better show fetching a single user so there is something easy to compare:

connect(props => ({
  userFetch: {
    comparison: props.userId,
    value: () => someAPI.getUser(`/users/${props.userId}`)
  }
}))(Users)

[ryanbrainard]

test name dupe. this looks like this was an accidential copy/paste of another test

[ryanbrainard]

What about changing to fetching a single user with a comparison on the props.userId so it's more obvious when the function is called again?

[ryanbrainard]

even though it won't actually change the behavior of the test, consider changing to value: () => { valueSpy() } to better test the function being created inside (like most apps would do), while still having the spy outside (so your test works)

[ryanbrainard]

@ipanasenko I just pushed up some changes to https://github.com/heroku/react-refetch/compare/stage-2.0.0. I updated the docs and tests as I mentioned in my review. If you're happy with them, we could merge them as-is (or of course push any changes you don't like). Then we can do the warnings (if we want them... interested to hear your thoughts) in a separate PR.

[slorber]

@ryanbrainard maybe it's worth issuing a warning in dev in case function change over time and comparison is not provided

[ryanbrainard]

I finally dusted this off and added the 'Request object with functional 'value' must also declare 'comparison' invariant to the stage-2.0.0 branch. I'll merge that into master shortly.

[ryanbrainard]

Published as beta v2.0.0-3

[ryanbrainard]

Published in v2.0.0