Skip to content

State: Avoid parameters in memoized selectors getDependants#4939

Merged
aduth merged 1 commit intomasterfrom
update/selector-dependants-param
Feb 8, 2018
Merged

State: Avoid parameters in memoized selectors getDependants#4939
aduth merged 1 commit intomasterfrom
update/selector-dependants-param

Conversation

@aduth
Copy link
Copy Markdown
Member

@aduth aduth commented Feb 7, 2018

This pull request seeks to resolve an issue where memoized selector cache clearing occurs more frequently than anticipated, due to the internal behavior of the underlying rememo library.

Given the following scenario, the cache for getBlock would be cleared on every single invocation:

for ( i = 0; i < 100; i++ ) {
	getBlock( state, i % 2 ? aBlockUID : bBlockUID );
}

This is because memoization compares the result of the dependents between invocations, and in the above case, blocksByUid[ uid ] will produce different results:

https://github.com/aduth/rememo/blob/11644c10d5bb6fa041c387c9ce9e6c26604d3556/es/index.js#L86-L90

As the maintainer of rememo, unless compelling arguments are made otherwise, I plan to release a subsequent major release dropping support for all but the state argument in getDependants (the second argument of createSelector).

Testing instructions:

Verify that unit tests pass:

npm run test-unit

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Feb 7, 2018
@aduth aduth requested a review from mcsf February 7, 2018 21:05
@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Feb 8, 2018

As the maintainer of rememo, unless compelling arguments are made otherwise, I plan to release a subsequent major release dropping support for all but the state argument in getDependants (the second argument of createSelector).

Sounds good, I can't really think of a compelling reason to use it.

Copy link
Copy Markdown
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Looks good, tests well, explanation clear. 🚢 !

@aduth aduth merged commit 61cf994 into master Feb 8, 2018
@mtias mtias deleted the update/selector-dependants-param branch February 8, 2018 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants