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.

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

Fix getInserterItems cache. #7055

Merged
merged 1 commit into from May 31, 2018

Conversation

Projects
None yet
2 participants
@jorgefilipecosta
Member

jorgefilipecosta commented May 31, 2018

The selector depended on the result of getBlockTypes() but it was not being taken into account on the creatSelector dependents.

cc: @aduth

Fix getInserterItems cache.
The selector depended on the result of getBlockTypes() but it was not being taken into account on the creatSelector dependents.
@aduth

aduth approved these changes May 31, 2018

@@ -1538,6 +1538,7 @@ export const getInserterItems = createSelector(
state.settings.allowedBlockTypes,
state.settings.templateLock,
state.sharedBlocks.data,
getBlockTypes(),

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

This gets me thinking: Elsewhere we depend on selectors who themselves are cached, we often duplicate those dependencies into the second argument set, which is why with rememo@3.0.0 I added a getDependants function to protect against such a piercing of abstraction. But, if we can assume that invoking the selector is at most only marginally less trivial than spreading in its dependants (as we're doing here), maybe that can also be a fine substitute?

It's a more complex problem here because getBlockTypes is not the selector itself, and we'd have no way to provide state to the selector form of getBlockTypes.getDependants.

Which is all to say: This is probably the correct approach here, but can / should we consider adopting this pattern more widely for composing memoized selectors?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 31, 2018

Member

Hi @aduth,
Here, unfortunately, getDependants is not an option because to the editor store we are just calling an external function.
Regarding the use of getDependants, the decision should be taken on a case by case basis, but in general I feel when the selector is cached we should use getDependants, the cases where we may call the selector directly when computing the dependents are when the selector is trivial and it is not cached so it makes sense to call it.

@jorgefilipecosta jorgefilipecosta merged commit 802f890 into master May 31, 2018

2 checks passed

codecov/project 46.47% remains the same compared to c93b778
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/getInserterItems-cache branch May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment