Selectors everywhere #2507

theengineear · 2018-02-08T04:15:26Z

In response to some comments @rgbkrk left in #2500 I used selectors in mapStateToProps. I also couldn't help myself and added a commit in here that updates some usage of mapDispatchToProps. I think the latter change is an overall improvement. However, I don't know how cleverly/simply the mapDispatchToProps arg is handled in connect. It could potentially be a perf issue to re-create all the functions in mapDispatchToProps... just wanted to draw that out.

Also, I think I found a test that was yielding false positives. You'll see it skipped in the diff. Here's an issue I opened about it:

#2506

Just to reiterate, the goal of these refactors are to ensure that our code doesn't depend on the structure of our state 🌲. We want to reorganize the state and add useful things like app refs to our state (see the plan.md file for details on that).

rgbkrk · 2018-02-08T17:00:16Z

packages/core/__tests__/providers/notebook-spec.js

-    test("handles a focus to next cell keypress on a sticky cell", () => {
+
+    // TODO: This test was silently broken. It was loudly found during a refact.
+    test.skip("handles a focus to next cell keypress on a sticky cell", () => {


Yeah, that's fine for now. We probably want to tear out the sticky cells at this point.

Not gonna tear them out in this PR, but I'm for doing it if we think that folks will not be terribly bothered.

Speaking of knowing if folks are terribly bothered, do we have any plans on trying to get anonymized usage data from folks? It'd be cool if we could see how many people are actually sticking cells. That's a whole can of worms, but I'm curious if it's been discussed?

Yes! We ran an informal poll on twitter in October of 2017.

Do you use "pin cell" in nteract desktop?

# % Choice

5 26.3% Never, it's so not for me

5 26.3% Once for funsies

7 36.8% Hardly ever

2 10.5% All the time, it's my jam

19 respondents

I'm willing to say, based on those results, that it would be ok to remove it in favor of maintainability. We can always keep the component + styling in core, just rip it out of the standalone notebook app.

rgbkrk · 2018-02-08T17:00:47Z

packages/core/__tests__/selectors-spec.js

+
+describe("codeMirrorMode", () => {
+  test("determines the right mode from the notebook metadata", () => {
+    // TODO: better way to get dummy state?


On some level, I think it's ok to only use the state we're actually using for the selectors.

Yah, the one thing I got hung up on was that it's common for use to use fromJS in tests, but that causes selectors to fail if they expect a Record and use . (dot) accessors. I changed all the selectors to use get and getIn, which is a pretty easy compromise, but It is a little odd. I guess records all have defaults though, so there's no reason you couldn't instantiate one in a test with only the bits you need filled in?

Ahhhhhh yeah. I suppose the records make it easier, which means using makeAppRecord and makeDocumentRecord should make building a dummy simpler than having to change dummy on the fly (or rely on its values).

rgbkrk

This is so fantastic! I made a lot of comments that I think are more for future reference, so I don't intend to block this. I'll go back and note which comments we should address first (and as part of this PR).

rgbkrk · 2018-02-08T17:02:21Z

packages/core/src/components/notebook-menu/index.js

-  filename: state.document.get("filename"),
-  notebook: state.document.get("notebook")
+  cellFocused: selectors.currentFocusedCellId(state),
+  cellMap: selectors.currentCellMap(state),


Refresh my memory, why does the notebook-menu need the whole cellMap and cellOrder?

It needs those because it needs to do runAllCells and such:

https://github.com/nteract/nteract/blob/master/packages/core/src/components/notebook-menu/extra-handlers.js#L19

BUT, if (when) we have an epic that does this for us... we can rip these out.

ooooooook

I'll get that into an epic since that would help both menu implementations

rgbkrk · 2018-02-08T17:03:18Z

packages/core/src/epics/contents.js

@@ -80,6 +80,12 @@ export function saveContentEpic(
      const state = store.getState();
      const currentNotebook = selectors.currentNotebook(state);

+      // TODO: this will likely make more sense when this becomes less
+      // notebook-centric.
+      if (!currentNotebook) {


We could / probably should emit an error rather than the empty.

Saving I definitely want to make sure we're propagating enough information back up, both for trust and transparency for users, and debuggability for us.

rgbkrk · 2018-02-08T17:03:37Z

packages/core/src/epics/contents.js

+      // TODO: this will likely make more sense when this becomes less
+      // notebook-centric.
+      if (!currentNotebook) {
+        return of(empty());


Aren't you able to return the empty() observable as is?

😬 ah, yah, you're right, this is returning an observable of an observable, so mergeMap will only shell off the outer one, leaving empty() instead of just er... nothing. thanks!

also i double-checked this (for sanity) and yes, just empty() should be returned.

rgbkrk · 2018-02-08T17:05:21Z

packages/core/src/epics/execute.js

+          if (!cellMap) {
+            return empty();
+          }
+


Since we're only using cell type and cell source here, I think the selector used above could be:

const source = cellSourceById(store.getState(), { id }); const type = cellTypeById(store.getState(), { id });

Updating the above comment to match the use of our cell ids (which of course are in fact refs).

Gonna do this after, but 💯

rgbkrk · 2018-02-08T17:06:41Z

packages/core/src/providers/editor.js

@@ -21,14 +22,14 @@ function mapStateToProps(
  state: Object,
  ownProps: CodeMirrorEditorProps
 ): Object {
+  const kernel = selectors.currentKernel(state);
+  const { cursorBlinkRate } = selectors.userPreferences(state);


Good old silly one configurable bit for codemirror here. 😝

rgbkrk · 2018-02-08T19:04:59Z

packages/core/src/selectors/index.js

+    if (notebook) {
+      return notebook.getIn(["metadata", "gist_id"]);
+    }
+    return "";


There's disparity here in that if ["metadata", "gist_id"] is not in the notebook, this will return undefined. If the notebook wasn't defined it returns "". I'd aim to return null in this one as:

notebook.getIn(["metadata", "gist_id"], null)

rgbkrk · 2018-02-08T19:06:12Z

packages/core/src/selectors/index.js

+    return toJS(notebook);
+  }
+  return null;
+});


I see what you mean by if we should throw here... If it's not set we should fail and know where it occurred. Do people normally allow for throwing in exceptions?

I'm not sure! I think flow should protect us in this case when we try to do anything with the selector, so i guess I'd rather return something and then have flow remind us that it could be null? I mostly say this because the error would likely originate somewhere in reselect code, which feels like it might be odd to debug?

I think returning null and handling it elsewhere is perfect, unless there's some sensible default (like with codemirror mode, etc.)

rgbkrk · 2018-02-08T19:11:19Z

packages/core/src/selectors/index.js

+    if (notebookJS) {
+      return stringifyNotebook(notebookJS);
+    }
+    return "";


⚠️ I'm a little afraid of us accidentally saving at some $PATH an empty notebook. I think I'm a fan of throwing here (or in stringifyNotebook). Perhaps the right choice though is to handle it in the epic -- if it returns null for instance, we should take a different path. I guess maybe that's the right choice though -- we do the handling in the epic where we have an ability to inform the UI and the user.

Cool. I think the moral here is to return null in these cases. I don't think I want to throw in the selectors...

rgbkrk · 2018-02-08T19:13:09Z

packages/core/src/selectors/index.js

+export const transientCellMap = createSelector(
+  (state: AppState) =>
+    state.document.getIn(["transient", "cellMap"], Immutable.Map()),
+  identity


This seems like one we'll want to do as a byRef setup 🔜, since no component ever needs all the cells.

rgbkrk · 2018-02-08T19:14:14Z

packages/core/src/selectors/index.js

+    return notebook.get("cellMap");
+  }
+  return null;
+});


I look forward to this being replaced by a selector which is cellByRef

rgbkrk · 2018-02-08T22:53:20Z

packages/core/src/epics/contents.js

+      // TODO: this will likely make more sense when this becomes less
+      // notebook-centric.
+      if (!currentNotebook) {
+        return of(saveFailed());


I do wish we provided more context here, so we know that it's because of a lack of notebook.

rgbkrk · 2018-02-08T22:58:00Z

packages/core/src/selectors/index.js

+    metadata.getIn(["language_info", "codemirror_mode"]) ||
+    metadata.getIn(["kernel_info", "language"]) ||
+    metadata.getIn(["kernelspec", "language"]) ||
+    CODE_MIRROR_MODE_DEFAULT


lock · 2018-05-01T10:05:57Z

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.

theengineear requested a review from rgbkrk February 8, 2018 04:15

rgbkrk reviewed Feb 8, 2018

View reviewed changes

rgbkrk requested changes Feb 8, 2018

View reviewed changes

rgbkrk mentioned this pull request Feb 8, 2018

[core] Sticky cells pass focus to next on shift-enter #2506

Closed

theengineear force-pushed the selectors-everywhere branch from ad891cc to 00f2705 Compare February 8, 2018 22:49

rgbkrk reviewed Feb 8, 2018

View reviewed changes

theengineear added 4 commits February 8, 2018 14:56

Udpate/Add more selectors 🎉!

b6ac2b0

Use selectors in mapStateToProps functions.

ecd1aae

Leverage mapDispatchToProps a little more.

1015138

Update specs. NOTE: ONE TEST WAS SKIPPED HERE.

c15c5f9

theengineear force-pushed the selectors-everywhere branch from 00f2705 to c15c5f9 Compare February 8, 2018 22:57

rgbkrk reviewed Feb 8, 2018

View reviewed changes

rgbkrk approved these changes Feb 8, 2018

View reviewed changes

rgbkrk merged commit 5fb299e into nteract:master Feb 8, 2018

rgbkrk added this to the Jupyter Extension M1 milestone Feb 10, 2018

rgbkrk mentioned this pull request Mar 7, 2018

nteract v0.8.0 release checklist #2645

Closed

7 tasks

lock bot locked and limited conversation to collaborators May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selectors everywhere #2507

Selectors everywhere #2507

theengineear commented Feb 8, 2018 •

edited

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018 •

edited

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk left a comment

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018 •

edited

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

theengineear Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

rgbkrk Feb 8, 2018

lock bot commented May 1, 2018

#	%	Choice
5	26.3%	Never, it's so not for me
5	26.3%	Once for funsies
7	36.8%	Hardly ever
2	10.5%	All the time, it's my jam

Selectors everywhere #2507

Selectors everywhere #2507

Conversation

theengineear commented Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgbkrk Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented May 1, 2018

theengineear commented Feb 8, 2018 •

edited

rgbkrk Feb 8, 2018 •

edited

rgbkrk Feb 8, 2018 •

edited