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

Fixes #23574 - add selector to facts charts using reselect #5778

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
6 participants
@amirfefer
Copy link
Member

amirfefer commented Jul 5, 2018

Reslect memorize derived data from the store, allowing redux to store the minimal possible state and improves performance.

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Jul 5, 2018

Issues: #23574

@amirfefer amirfefer changed the title Fixes #23574 - add reselct to facts charts Fixes #23574 - add selector to facts charts using reselect Jul 5, 2018

@amirfefer amirfefer force-pushed the amirfefer:23574 branch from 9b5a6d3 to cd1a6b6 Jul 5, 2018

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Jul 5, 2018

@amirfefer looks good :) @sharvit, @ekohl ack?

@amirfefer amirfefer force-pushed the amirfefer:23574 branch from cd1a6b6 to aa65bcc Jul 5, 2018

@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Jul 8, 2018

@amirfefer I really like it!

I think we should discuss about strategies and naming conventions for selectors.

This is what I came with:

  1. Selectors file should name with Selectors suffix (e.g. SomethingSelectors.js)
  2. Need to export a base none-memorized selector that only select the relevant state in the state tree.
  3. All the selectors need to be based on the none-memorized base selector
  4. Selectors should name with select prefix (e.g. selectFactChartData, selectFactChartHostCounter)

Example:
https://gist.github.com/sharvit/bd8ed40f9d1d7a2ed74b2bb572876e23

I'n not sure about the selectFactChartData, should it be memorized?

@ik5
Copy link
Contributor

ik5 left a comment

I think that selectors should have their own directory.
that is wepack/assets/javascript/react_app/selectors.

Inside it can be either by sub directory for a subject, or just the name of the selector.

The reason for it is to easier find and understand what belongs to where, even in a year from now, instead of looking just at the charts component for example.

at least my opinion in the matter.

@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Jul 9, 2018

@ik5 we went into this way before with the actions and the reducers and it feelt difficult to develop and maintain in this way.
We made a decision to switch into a domain level development, we liked this article:
https://medium.com/@alexmngn/how-to-better-organize-your-react-applications-2fd3ea1920f1

@ik5

This comment has been minimized.

Copy link
Contributor

ik5 commented Jul 10, 2018

@sharvit okay, let's flow with that blog post, your structure is still not as it was suggested.

You went for a structure with actions and reducers, and now you are breaking it, and doing so with no order at all, and that render the ability to maintain code for someone that does not live in 24/7 and now exactly what to expect every second a nightmare.

Also, take in consideration that even you, will not remember every such decision for long term. That is, you might remember that you created something, but not where it is, and what was the function name, or file name, and now what?

So when going on a structure, regardless of what the structure is, it is better to keep up with it, for not having a mess.

Think of Principle of least astonishment. It is okay to have "components" -> "actions", but if you do it for some, and not for all, it's a mess.

The division is less a concern, but components cannot be a root for everything else, it is driven by context.

So we should use a foreman domain driven structure, for example the usage of that chars, and then sub directory for selectors for example, so it will be easier to figure it out.

My 2 cents on the matter.

@@ -90,6 +90,7 @@
"redux-form-validators": "^2.1.2",
"redux-logger": "^2.8.1",
"redux-thunk": "^2.2.0",
"reselect": "^3.0.1",

This comment has been minimized.

Copy link
@ohadlevy

This comment has been minimized.

Copy link
@ohadlevy

This comment has been minimized.

Copy link
@ekohl

ekohl Aug 13, 2018

Member

Should be be OK to add. I'll wait with submitting the actual PR because in the past those were open for a very long time and I have closed some because it took too long. Looks like this one is a trivial ./add_npm_package.sh reselect in our packaging repo.

This comment has been minimized.

Copy link
@ohadlevy

ohadlevy Aug 13, 2018

Member

there are multiple PRs that have this dependency, so I would think its safe to add.

This comment has been minimized.

Copy link
@ekohl

ekohl Aug 13, 2018

Member

It won't be used until foreman.spec actually requires it and that's based on package.json so when one of the PRs that adds it is approved I'll open a PR.

@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Jul 10, 2018

@ik5 I can totally understand why you feel it is a nightmare and we need to better in this field.

  • We made the decision to write new entry level components as described but we are not dedicated enough to do a full migration 👎
  • We never documented it or wrote a tutorial about it 👎

I don't think we should move the selectors to CompnentName/selectors/index.js.
It is make sense to me if we end up with a huge ComponentName/ComponentNameSelectors.js to split each selector to individual files, put them under ComponentName/ComponentNameSelectors/selector1..n.js and export them all from ComponentName/ComponentNameSelectors/index.js.

@ik5

This comment has been minimized.

Copy link
Contributor

ik5 commented Jul 11, 2018

@sharvit I'm not sure that placing a selector under a component is a right way.
The component should be used more then once.

So having a scope, for example dashboard -> components -> ComponentName should be easier to do.
And then under the dashboard to place a selector will have more logical way of doing things, so it can also be: dashboard -> selectors, and that way you know where everything is coming from, and what belongs where.

If now you'll want to reuse just this component, but with new data, you actually made a more complex move, because you are tying yourself to that selector inside that component.

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Jul 22, 2018

@amirfefer can you rebase please?

@amirfefer amirfefer force-pushed the amirfefer:23574 branch from aa65bcc to 1f202f5 Jul 22, 2018

@amirfefer

This comment has been minimized.

Copy link
Member Author

amirfefer commented Jul 22, 2018

@sharvit - could you have another look on the conventions?

: 0),
);

export const selectFactChart = state => state.factChart;

This comment has been minimized.

Copy link
@sharvit

sharvit Jul 24, 2018

Contributor
  1. Does it make sense to reuse the selectFactChart in values?
  2. Does it make sense to export the values selector under selectFactChartData?
export const selectFactChart = state => state.factChart;
export const values = state => selectFactChart(state).chartData;

I think it helps to have a single responsibility for each part of the state.

import { hostCounterSelector } from './FactChartSelector';
import { chartDataValues } from './factChart.fixtures';

describe('Fact Chart Selector', () => {

This comment has been minimized.

Copy link
@sharvit

sharvit Jul 24, 2018

Contributor

Do we need to test selectDisplayModal and selectFactChart?

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Jul 25, 2018

@amirfefer note the test failures too.

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Aug 6, 2018

@amirfefer whats the status of the PR? thanks (I'm seeing at least two more PR's that use reselect) - wondering which one should go in first?

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Aug 13, 2018

@amirfefer bump?

1 similar comment
@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Oct 2, 2018

@amirfefer bump?

@amirfefer amirfefer force-pushed the amirfefer:23574 branch from 1f202f5 to ef4ca03 Nov 5, 2018

@theforeman-bot theforeman-bot added the UI label Nov 5, 2018

@amirfefer amirfefer force-pushed the amirfefer:23574 branch from ef4ca03 to 72f99ca Nov 5, 2018

@amirfefer

This comment has been minimized.

Copy link
Member Author

amirfefer commented Nov 5, 2018

@sharvit could you have another look please?

@sharvit

sharvit approved these changes Nov 6, 2018

Copy link
Contributor

sharvit left a comment

Thanks @amirfefer LGTM 👍

@ohadlevy ohadlevy merged commit ade079c into theforeman:develop Nov 6, 2018

7 of 8 checks passed

foreman Build finished. 36683 tests run, 5 skipped, 2 failed.
Details
Hound No violations found. Woof!
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on 23574 at 83.192%
Details
katello Build finished. 4073 tests run, 9 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished. No test results found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.