work() should get the exact props passed to the component

[smirea]

right now work() has access to the 2 internal methods of withJob and does not have access to the job state itself.

It's probably debatable whether the work() method should have access to the job state, but I guess it could be useful for the cases where you want to perform logic depending on where the job is inProgres or not (e.g. cancel in-flight jobs if you're using a .then-able observer or something)

[ctrlplusb]

Woah, interesting case! Do you have an example of how you may use this?

[ctrlplusb]

I've added you as a collaborator! We will have to update the package.json with your details. I appreciate your interest and help here!

[smirea]

thanks!

My main gripe is that work() seems to do a bit more than i'd like and I want to abstract away some complexity - specifically on data refetching.
Most of the time I find myself using this pattern in components that only need data once and then they're good - a prime example are page components passed directly to react-router:

<Match pattern='/foo' component={Foo} />

@withJob(props => 
    fetchPage(props.id)
)
class Foo extends PureComponent {}

The above pattern only needs to load data once in its lifetime. If the route changes the components gets unmounted and when the route goes back to /foo a new instance is created and fetch() can re-run. Actually pretty much all of the components that I can think of that need init async data, need it only once in their lifecycle.

As such, I actually want my default interaction to behave exactly like the simple caching example: https://github.com/ctrlplusb/react-jobs#simple-caching-of-asynchronous-job

I'm a big proponent of building abstractions that give you as few ways of shooting yourself in the foot as possible, so instead of relying on a convention to always do the above, I want to abstract the functionality in a wrapper method:

import {withJob} from 'react-jobs/ssr';

// Only fetch if there is no job in progress and the job isn't done
const defaultFetch = ({job}) => !!(!job || (!job.completed && !job.inProgress));

const DataLoader = ({fetch, shouldFetch=defaultFetch}) => Component => {
    let cache = null;
    return withJob(props => {
        if (!shouldFetch(props)) return cache;
        return fetch(props).then(result => {
            cache = result;
            return result;
        });
    })(Component);
};

The above abstraction does not care what you do with your data, if you store it in a redux store or somewhere else, or what its prop name is (which is a caveat of the caching example). It does not allow you, by default, to refetch and it also prevents you from importing react-jobs instead of react-jobs/ssr. But the final piece of the puzzle is the missing job state which is the generic arbiter needed. Then extra generic features can be added around it, like retrying etc

now, the above becomes:

import DataLoader from 'data-loader';

@DataLoader({
    fetch: ({route: {params: {id}}}) => fetchPage(id), // <-- Will only ever get called once
})
class Foo extends PureComponent {}

... come to think of it, do you have an use-case for data-refetching that's more common than a one-time use per lifecycle?

[ctrlplusb]

That is great and I am so glad that you are seeing the potential for abstraction as this was a big part for me in creating the functional API. 😀

In regards to executing the work on mount and possibly again on receiving props. This is actually a bug come to think of it. We need to amend the componentWillReceiveProps to ensure that the work is only executed if work isn't already in progress and the work hasn't been completed already. Good spot!

[smirea]

but the question is, do you want to allow for re-fetching of data at all? I've used things like redux-async-connect before which does not give you the option of re-fetching and I've never actually noticed it missing as I haven't come across an use-case for it

[ctrlplusb]

I think if you aren't connecting to a state management system then there is sufficient need for the job to fire again. Its easy enough to write your own wrapper to prevent this case of course, but then it can be a choice. I want it to be a flexible abstraction.

[ctrlplusb]

@smirea I have done a 0.4.3 release. With the fixed semantics do you think you will still have need for the job prop being passed into the work?

To be honest I am a bit opposed to it at the moment. It feels a bit confusing to myself, but I am open for discussion. 😀

[smirea]

awesome!

I think now this becomes a bit too inflexible since you're effectively enforcing that work() will only be called once [1] [2] (since the second it's called the state will be either inProgress or completed). For my personal use-case this is ok, however things like retrying are not possible anymore since once you failed you're in a state of {completed: true} so handleWork() won't be called anymore

[1] technically setState() is async so I'm not 100% sure if there is a path where handleWork() can be called multiple before the state is updated. Might want to use a class property as a mutex flag
[2] also, the cache example now does not make sense anymore, since the only valid state for handleWork() to execute is the initial state

[ctrlplusb]

however things like retrying are not possible anymore

A very interesting point! I shall think on this one for a while. Thanks!

technically setState() is async so I'm not 100% sure if there is a path where handleWork() can be called multiple before the state is updated

setState is sync when executed in the componentWillMount, so for async work the state will initially be "inProgress", whilst sync work will be "completed".

Then when any prop updates come from a parent the work could be fired again unless we check for these conditions.

also, the cache example now does not make sense anymore, since the only valid state for handleWork() to execute is the initial state

If the component is unmounted then mounted again then it would fire again. For very simple apps this makes sense. E.g. a component per "page" that fetches it's data every time you browse to the page.

--

The failure/retry case is very interesting though! I think we should consider adding an option to the withJob function that would allow you to describe behaviour for this case. Let's give it some time and see what we can come up with.

Thanks so much for all your sense checking! I really appreciate it. 🙏

[smirea]

sounds good.

do you think you will still have need for the job prop being passed into the work?

it's still nice to have it just so you have the exact same props in both cases. not sure what other tactics can be performed down the line if this API is exposed, but I don't necessarily see any reason against exposing it, since it's passed down to the child component anyway.

relevant side-note: at the very least remove the 2 internal props that are passed :)

[ctrlplusb]

relevant side-note: at the very least remove the 2 internal props that are passed :)

Done in release 0.4.0 🙏

[smirea]

they were removed from being passed to the child, but they are still passed to work() since you just pass down props: https://github.com/ctrlplusb/react-jobs/blob/master/src/withJob.js#L61

[ctrlplusb]

Aha! Thanks again. You have been awesome at spotting these issues for me!