Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proof of concept for reselect without connect #9

Open
wants to merge 1 commit into
base: memoize-functions
Choose a base branch
from
Open

proof of concept for reselect without connect #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions containers/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import { addTodo, completeTodo, setVisibilityFilter, changeTheme, VisibilityFilt
import AddTodo from '../components/AddTodo'
import TodoList from '../components/TodoList'
import Footer from '../components/Footer'
import { memoize, createMemoizedFunction } from '../memoize'
// import { memoize, createMemoizedFunction } from '../memoize'
import { createSelector } from 'reselect'

function selectTodos(todos, filter) {
console.log("Recalculating selectTodos");
Expand All @@ -23,14 +24,27 @@ function selectMatchingTodos(todos, search) {
return todos.filter((todo) => { return todo.text.search(search) >= 0; });
}

const visibleTodosSelector = createSelector(
[(component) => component.props.todos, (component) => component.props.visibilityFilter],
selectTodos
);

const matchingTodosSelector = createSelector(
[(component) => component.visibleTodos(), (component) => component.state.search],
selectMatchingTodos
);

class App extends Component {
constructor(props, context) {
super(props, context);
this.state = { search: '' };
}

visibleTodos = createMemoizedFunction(() => [this.props.todos, this.props.visibilityFilter], selectTodos);
matchingTodos = createMemoizedFunction(() => [this.visibleTodos(), this.state.search], selectMatchingTodos);
// visibleTodos = createMemoizedFunction(() => [this.props.todos, this.props.visibilityFilter], selectTodos);
// matchingTodos = createMemoizedFunction(() => [this.visibleTodos(), this.state.search], selectMatchingTodos);

visibleTodos = visibleTodosSelector.bind(this, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strikes me as undesirable because visibleTodos is required to be here for for this class to work, but it's not used directly anywhere inside this. If visibleTodosSelector was defined in a different file and/or if multiple connected components used that selector, it would become very hard code to follow (and probably maintain):

To generalize that- with this approach, looking up the chain of dependent selectors, you go up the chain and eventually the dependencies of a "top level" selector are the props of [every component that uses the selector]. With a selector that's not a top level selector, like matchingTodosSelector, you have the dependency that [every component that uses the selector] defines visibleTodos as a property, even though it's not used directly, or passed directly to anything in class App.
Or:

// Reselect without connect:
# of places to look/check for selector dependency like visibleTodos() === # of components that use any selectors that depend on visibleTodos()

This is in contrast to regular connected reselect, where each selector's dependencies are clearly defined in one place (even if they're not in the same file, they're still in one place, and where they come from if in a different file is explicated at the top of the file in the import statement). For the "top-level" selectors, they only have one dependency, the state, and that dependency is super easy to watch change, and we always know where it is.
Or:

// Regular connected reselect
# of places to look/check for a selector dependency === always 1 (either state, or other explicitly defined and imported selector)

Basically I think a lot of complexity is added in going from:

// Regular connected reselect
state --> denormalization --> component consumption

to:

// Reselect without connect:
state --> component --> denormalization --> component consumption

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your points are helping to distinguish a difference between global selectors for denormalizing state that components would want to receive as denormalized vs. computations that a component may want to memoize.

This PR proved you could use reselect as the memoizing engine for component-specific calculations, but it's really serving a different purpose within the app than a global selector.

Let me explain:

Suppose the redux store has a list of specialties, and we want to transform those into a different data structure for easier consumption by a <Select> component Also suppose two different components need the specialties options, say <NewReferralPage> and <NewServiceRequestPage>. I think it could make sense to use a reselect selector to denormalize the data globally, and then use connect (once for each page component) to pass the denormalized data into each of those two pages as a property like specialtyOptions.

On the other hand:

Now suppose we add <Pane> components to the todo app, with each pane having its own searchTerm and visibilityFilter. Each pane should take three properties: todos, filter, and searchTerm. Each pane needs to combine those properties into the derived data that the pane wants to display. Computing this data in a memoized fashion is important, and this PR shows how that could be done by leveraging the reselect library, even though it's serving a different purpose than a global selector.

In many cases, we may have a component that needs to make calculations, and we may want to memoize those calculations. If there is only one kind of component that needs such computations (for example, a todo pane is the only kind of component that needs to know about visibleTodos and matchingTodos, there isn't much value in defining a reusable selector that knows how to reach into the component to find the inputs.

However, if there are multiple kinds of components that need to perform the same kind of computations on the same kind of inputs, then we could extract the logic for "finding" the inputs within the component into a reusuable module ---- in such cases, we're building an implicit contract for what methods those components need to expose. This is completely analogous to our Personable module in Ruby, where the implicit contract is that the class must expose a first_name and a last_name method:

module Personable
  extend ActiveSupport::Concern

  def name
    "#{first_name} #{last_name}"
  end

  def comma_delimited_name
    "#{last_name}, #{first_name}"
  end
end

Now each class that wants to use the Personable module has complete flexibility to define first_name and last_name however it wishes, but the Personable module is completely agnostic about that. This is analogous to matchingTodosSelector being agnostic about how a component gets or provides a visibleTodos() method.

As for counting the number of places to look for a dependency --- I'm not sure I follow what you're trying to say.

It's true that a global selector intending to transform state data can "live" anywhere and be "connected" anywhere, without worrying about whether the component that wants to use it fulfils a certain implicit contract. This is because the actual "implicit" contract of such selectors is that they be called by something (connect) that provides a particular object (state).

It's also true that a selector like matchingTodosSelector() is a different animal because it is expected to be passed a component that provides a certain API. Therefore, not just any component can use it (in the same way that not just any model in our Ruby app would use Personable).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically when I was describing "number of places to look for a dependency" I was talking about the number of spots we need to be sure conform to that implicit contract (exposing this.props.todos and this.visibleTodos()). I think relying on those implicit contracts is undesirable in this case, because it makes testing the functions that compute visibleTodos and matchingTodos more difficult to test (they would arguably need to be tested in the context of every class/component that include them), and because it makes the code/thing/function that computes visibleTodos tightly coupled to the components that want visibleTodos computed (the computing function needs knowledge of how the objects passed to it expose the data it needs, not to mention it expands the component's public API).

I think it would be better to have "computations that a component may want to memoize" implemented as memoizing functions that don't need to know anything about the classes/components that use them (no implicit contracts, instead a more explicit contract in the form of args). The memoizing function would just take the pieces of data that are necessary for the computation as arguments, instead of binding/passing the function this, and expanding this's public API.

So instead of:

visibleTodos = visibleTodosSelector.bind(this, this);

it could be something more like:

visibleTodos = visibleTodosFactory(todos, visibilityFilter); 
// Note: for this to be a memoizing function it would actually end up being something more like: visibleTodosFactory([todos, visibilityFilter]), but I don't want to de-rail

The benefits of trading the passing-of-this for passing-in-args approach here are:

  • Looser coupling
  • Easier to test (both the component, and the code that computes the visibleTodos)
  • More explicit contract (code easier to follow and reason about)
  • No public API expansion of component class
  • More idiomatic

Here is a blog post related to includeing utility functions and creating higher order components as means of achieving composition.
https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750

NOTE: it turns out lodash has a nice function for creating memoizing functions: const square = _.memoize((a) => { return a^2; });. For each value of a, square(a) will only be computed once, and I think is roughly the same API we briefly thought might be possible with reselect.

matchingTodos = matchingTodosSelector.bind(this, this);

updateSearch = function(e) {
this.setState({ search: e.target.value });
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
"react": "^0.14.0",
"react-dom": "^0.14.0",
"react-redux": "^4.0.0",
"redux": "^3.0.0"
"redux": "^3.0.0",
"reselect": "^2.0"
},
"devDependencies": {
"babel-core": "^5.6.18",
Expand Down