More selectors #2500
More selectors #2500
Conversation
|
| @@ -8,7 +8,7 @@ import { | |||
|
|
|||
| import { overwriteMetadata, deleteMetadata } from "@nteract/core/actions"; | |||
|
|
|||
| import { toJS, stringifyNotebook } from "@nteract/commutable"; | |||
| import * as selectors from "@nterat/core/selectors"; | |||
| notebook.getIn(["metadata", "github_username"]) !== | ||
| (res.data.login || undefined) | ||
| ) { | ||
| if (githubUsername !== (res.data.login || undefined)) { |
Cool. Accepted 😎. |
We will commonly be naming variables with the same name as the selector. Instead of doing variable gymnastics, we can just import selectors in a namespace.
You're always *getting* with selectors, so this is a little excessive.
theengineear
force-pushed
the
more-selectors
branch
from
8eb3e3f
to
3dcdfc7
Compare
| dummyCommutable, | ||
| dummy, | ||
| "fake-github-username", | ||
| "fake-gist-id", | ||
| "./test.ipynb", | ||
| notificationSystem, | ||
| false, |
There was a problem hiding this comment.
This should all make @jdetle happy! 🎉
| const serverUrl = (state: AppState) => state.app.host.serverUrl; | ||
| const crossDomain = (state: AppState) => state.app.host.crossDomain; | ||
| const token = (state: AppState) => state.app.host.token; | ||
|
|
||
| export const getServerConfig = createSelector( | ||
| export const serverConfig = createSelector( |
packages/core/src/selectors/index.js
Outdated
|
|
||
| const identity = thing => thing; |
packages/core/src/selectors/index.js
Outdated
|
|
||
| import { createSelector } from "reselect"; | ||
| import { toggleCellInputVisibility } from "../../actions"; |
There was a problem hiding this comment.
Looks like this didn't mean to be here
| (hostType, kernelType) => { | ||
| return hostType === "jupyter" && kernelType === "websocket"; | ||
| } | ||
| ); | ||
|
|
||
| // TODO: if we're not looking at a notebook in the UI, there may not _be_ a | ||
| // notebook object to get. Do we return null? Throw an error? |
There was a problem hiding this comment.
That or emptyNotebook (from commutable)
| export const currentNotebookGithubUsername = createSelector( | ||
| [currentNotebook], | ||
| notebook => notebook.getIn(["metadata", "github_username"]) | ||
| ); |
There was a problem hiding this comment.
Since we're having to put the selectors here for the github functionality, we might as well move the reducer from desktop's app reducer (and maybe the epic...). Not for this PR, but it'll be nice for desktop to have no custom reducers now.
There was a problem hiding this comment.
ah, didn't even realize that these were being set in desktop.
| .skip(index) | ||
| .filter(id => cellMap.getIn([id, "cell_type"]) === "code"); | ||
| } | ||
| ); |
There was a problem hiding this comment.
At some point we'll want to re-evaluate how we do run-all and run-all-below, since they belong in an epic that tracks the execution rolling on through.
| return action$.pipe( | ||
| ofType(SAVE_AS), | ||
| mergeMap(action => { | ||
| const state = store.getState(); | ||
| const notebook = state.document.get("notebook"); | ||
|
|
There was a problem hiding this comment.
Ooooohhhh cool, we didn't need the notebook here anymore.
|
Separately, we should extract |
|
We'll probably want to switch a lot of our |
theengineear
force-pushed
the
more-selectors
branch
from
3dcdfc7
to
084764a
Compare
| state.document.get("notebook"), | ||
| state.document.get("savedNotebook") | ||
| selectors.currentNotebook(state), | ||
| selectors.currentSavedNotebook(state) | ||
| ); |
There was a problem hiding this comment.
This could be an isSaved selector.
|
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. |
Jumping on the selectors train! I want to start moving state around, but I won't be confident about doing that until we centralize access with these selectors.
Couple questions:
getCurrent*, batman! It's a little wordy! Maybecurrentshould be the default thing we assume to be getting when we're using selectors.notificationStystemstuff: not touching it, it's like an object with methods on it? I'm just going to assume we're not moving that :0getCurrentCodeCellIdsBelow(i know, a mouthful...). you're on board for more of the same, right?