That's so fetch! #2541
That's so fetch! #2541
Conversation
| ) => | ||
| action$.pipe( | ||
| ofType(actionTypes.SET_NOTEBOOK), | ||
| map((action: SetNotebookAction) => { |
There was a problem hiding this comment.
I gotta remember to set the type here! Nice.
There was a problem hiding this comment.
Pretty handy since we don't have the rxjs operator types via flow. 馃槃
packages/core/src/epics/contents.js
Outdated
| @@ -114,6 +114,7 @@ export function saveContentEpic( | |||
| } | |||
|
|
|||
| // When content gets loaded, if it's a notebook, set it up as the notebook | |||
| // TODO: should this be renamed as "fetchContentFulfilledEpic" | |||
There was a problem hiding this comment.
Potentially. Is there a case when we have multiple epics listening for the same actions? If not, it's sorta nice when the naming lines up.
There was a problem hiding this comment.
There won't be more than one epic listening for this (I think).
There was a problem hiding this comment.
Then, "yes". 馃槃 but you don't need todo that in this pr.
| @@ -117,41 +110,53 @@ function createContentsResponse( | |||
| */ | |||
| export const loadEpic = (action$: ActionsObservable<*>) => | |||
| action$.pipe( | |||
| ofType(actionTypes.LOAD), | |||
| ofType(actionTypes.FETCH_CONTENT), | |||
There was a problem hiding this comment.
Cool, so the load action is gone now, and we just hook up this epic in place of the core fetchContent epic. I wonder if we should actually call this the fetchContentEpic now so that it's clear that we're using the desktop-specific fetchContentEpic, not the core fetchContentEpic?
There was a problem hiding this comment.
I like that, I'll switch the name up here.
|
Woo, I fixed the tests! On to addressing feedback. |
| type: actionTypes.FETCH_CONTENT_FULFILLED, | ||
| error: Error | ||
| }): FetchContentFailed => ({ | ||
| type: actionTypes.FETCH_CONTENT_FAILED, |
There was a problem hiding this comment.
馃憣 ha, i totally had this in a stashed change locally. you beat me :)
| actions.launchKernel(action.kernelSpec, action.cwd) | ||
| ) | ||
| ) | ||
| switchMap(action => { |
There was a problem hiding this comment.
This is just synchronously returning an action, right? Couldn't we just map here?
|
Howdy! I'm 馃敁馃! In order to keep information timely (based on the most recent release), we want all activity to be added to either new issues or open issues and PRs. In service to that goal, I, the lock bot close inactive closed issues when they haven't had activity in 120 days. Feel free to open a new issue for related bugs and link to relevant comments from this thread. |
Desktop is now just like the webapp, in that it creates Contents API responses! No more
loadaction orLOADactionType. 馃槃This also fixes up
fetchContentFailedreturning the right payload now.All of this is in support of making #2516 easier to digest.