[TM-1531] add job connection #745
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/connections/DelayedJob.ts
Outdated
| hasLoadFailed: bulkUpdateJobsFetchFailed(store) != null, | ||
| response: bulkUpdateState.response | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This should use ?? for defaults, although I think this is an overall better format for this method:
const bulkUpdateJobsSelector = (store: ApiDataStore) => ({
isLoading: bulkUpdateJobsIsFetching(store),
hasLoadFailed: bulkUpdateJobsFetchFailed(store) != null,
response: store.delayedJobs?.reponse
});
However, I'm also confused as to why you would expect a response object to be on store.delayedJobs. That shouldn't be possible with the current redux setup.
There was a problem hiding this comment.
the response object wasnt needed, and everything keeps working.
src/connections/DelayedJob.ts
Outdated
| } | ||
| }, | ||
|
|
||
| isLoaded: state => !state.isLoading, | ||
|
|
||
| selector: createSelector([store => bulkUpdateJobsSelector(store)], ({ isLoading, hasLoadFailed, response }) => ({ | ||
| selector: createSelector(bulkUpdateJobsSelector, ({ isLoading, hasLoadFailed, response }) => ({ | ||
| isLoading, | ||
| hasLoadFailed, | ||
| response |
There was a problem hiding this comment.
This seems like it should be throwing an error in the TS compiler... Maybe I'm missing something, but this shape does not seem to match DelayedJobsConnection.
A couple of thoughts here:
- Typically, each connection will have its own shape. That's not required, but it's normal
- I've found in the past that properties like
isLoading,hasLoadFailedandresponse, while seeming convenient when looking at a single connection, are actually a disaster down the road. The reason being that if we follow such generic conventions for individual connections, then when a component ends up using 2 or more connections, it gets really awkward to juggle all theisLoadings that each connection returns. Instead, I think it's better to be super specific with the properties of the connection shape. So something more likebulkUpdateJobsIsLoadingandbulkUpdateJobsHasFailed.
As for response - this appears to be just returning all the delayed jobs like the other connection does. Are these used in the same component? Perhaps a better approach would be to combine them into a single connection. I think of connections as being responsible for providing a given set of data from the redux cache, and a set of functions for operating on that data. It would be totally reasonable for this bulkUpdateJobs call to be triggered from a function that's included on the other connection. Or, the bulkUpdate function could be simply exposed by this file, and then we simply rely on the original connection to provide the delayedJobs from the store. One of the primary wins of this system is that a connection returns data that got cached in the store regardless of where it came from. So if one call does the initial fetch, and another call modifies that data, as long as the data ends up back in the store under the same ID, the component that relies on it will rerender when it gets updated.
add the connection to pull the list of jobs