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.

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

Store: Avoid Lodash get for getPostEdits #7381

Merged
merged 1 commit into from Jun 20, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 19, 2018

This pull request seeks to refactor the getPostEdits selector to avoid relying on Lodash's _.get. The _.get function includes some overhead, and there is never a case in the runtime application where the state.editor.present.edits value does not exist, since it is part of the accumulated reducer shape. It was used only for convenience in tests, which is not a justifiable reason to introduce the overhead.

Testing instructions:

Verify unit tests pass:

npm run test-unit editor/store/test/selectors.js

@aduth aduth added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Jun 19, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

This makes sense and is a definite improvement.

It highlights the risks of creating a raw state object in tests rather than dispatching actions to create a usable state. We can into this a lot on the addons-frontend project and created test helpers to initialise actual states rather than making up fake ones.

Is that something we could do for Gutenberg's tests? If yes I'll file a code quality issue, I just want to make sure it wasn't avoided for a technical reason...

@aduth
Copy link
Member Author

aduth commented Jun 20, 2018

@tofumatt This is an excellent point. It's come up a few times informally in the past. I agree it's not great that we have to manually craft the state shape when testing selectors, and it's been particularly painful when that state shape changes (and its resulting impact on selector tests).

I'd considered something similar to what you propose, where we could effectively just run the store reducer and merge in the tree paths (via _.merge) the values relevant for the test case.

On the other hand, the one benefit to manually crafting state shape is that if in-fact these selectors depend on parts of the state shape existing, even if indirectly, it's important that these are considered in their own test cases. For example, it can be argued that our unit tests for isPermalinkEditable are incomplete because they do not include a case where an edits value for permalink_template takes precedent over the one for the saved post (by its use of getEditedPostAttribute). Conversely, if we don't want the edited value to be considered and instead only that of the saved post, the selector should use getCurrentPost instead of getEditedPostAttribute. This becomes obvious here only in not allowing the state.editor.present.edits path to be unreachable.

I agree it'd be good to create an issue to discuss this further.

@aduth aduth merged commit 657c368 into master Jun 20, 2018
@aduth aduth deleted the remove/get-post-edits-get branch June 20, 2018 12:23
@mtias mtias added this to the 3.1 milestone Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants