Skip to content

Conversation

@davidcurras
Copy link
Collaborator

No description provided.

@tribou tribou temporarily deployed to tribou-react-template-pr-59 November 27, 2017 18:36 Inactive
@olivoil
Copy link
Contributor

olivoil commented Apr 3, 2018

I like the grouping of state-related abstractions into more logical components, I think that's mostly the intent of the PR, right?

Here are a few notes and questions after reading through the code:

  • could we move selectors and epics into these redux modules? It seems it would make sense to group them together with other logical parts of the state?

  • some of the folders under redux/modules have empty files like redux/modules/env/consts.js or redux/modules/env/actions.js. Could they be deleted? No need to clutter our minds with something that isn't needed yet. I think it's preferable to create the file only when you need it, and not in advance as a placeholder for something you may or may not need.

  • looking at redux/modules/auth, things are split up into tiny files right now. It seems disproportionate with the actual size of the module, making it more difficult to hold a mental model of the whole thing. So I think the const, actions, and reducers files should be merged together in this example. If the example grew bigger, it might make sense then to split things up further to make it easier to reason with, but I wouldn't do it before it becomes a necessity. Instead of splitting things up too early, maybe we could document the ideal organization of a large module into a readme file? It would also help onboarding anyone new to redux by explaining each abstraction?

  • what do the env, request, init, and ui modules do? Looking at them from a newcomer's point of view, it's not immediately clear to me how they are supposed to be used, if at all. Is there a way to make their purpose clearer? Or remove them if they are not actually useful?

@tribou
Copy link
Owner

tribou commented Apr 3, 2018

I can clarify the last point on explaining the modules:

  • env is a placeholder "pass-through" reducer to be able to inject the Node.js runtime environment variables on each new page request. The env vars get passed in the server request and rehydrated on the client from the initial state that gets passed to the client-side store. In redux, you just have to have a reducer already defined for each property that you pass to the store. So even though we're not ever changing the env vars on the client, we still need an empty reducer to be able to do this. However, I just remembered that in order to get import env from 'config/env working properly in the browser, I had to setup a global window property that is used to hold the env properties separate from the store (so that you could import them dynamically in any file). Consequently, we may be able to switch to using import env... wherever needed and remove this reducer from the project.
  • request is another placeholder "pass-through" reducer that allows us to pass the user-agent and any other request headers or properties from the server that we might want to use. It's not best practice to use user-agent parsing in the browser, so this could probably be removed for most projects. It was needed in an edge-case once, and I added it to the template as an example reference.
  • ui is a place to store any global UI-related state. Displaying a global loading indicator or capturing whether a main layout sidebar is open or closed (that might need to push content out of the screen in other components for mobile views) are two examples of when this could be used.
  • init is my very first redux reducer 😛 It was originally used to capture page loading and "is loaded" events to be able to show a splash screen while the UI and images were loading and then fade it out cleanly once all the content was there. This was before server-side rendering, so now we have a lot more flexibility even without the init reducer. It could be removed unless we wanted to create an "everything did render and fetch" type of hook.

@olivoil
Copy link
Contributor

olivoil commented Apr 3, 2018

thanks for the clarification. So, what could be a good next step about these modules?

  • remove env, request, and init from the template as they are not needed?

  • as for ui, it seems it's an example of how to implement the state of generic UI components, maybe we should move it to the examples folder then and use it in an example component so it's more cohesive with the rest of the template? Is ui a good redux module name for this, or should it be something like shared to mirror how react component that are generic pieces of UI are structured?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants