React Wrapper Step 6: Reduxify App Lab#7148
Conversation
Introduces the 'envify' transform into our apps build. When MOOC_DEV=0 (as in npm run build:dist) we add the envify transform to our browserifyinc command and set NODE_ENV to "production". This replaces any instance of process.env.NODE_ENV in our own code with the string "production" which allows us to set some conditional behavior according to the enviornment. Uglify then removes now-dead code paths in minified production builds. In this first case, we check the environment to build a redux store with no middleware for production builds, but introduce middleware for logging state changes and connecting to the Redux Chrome DevTools in debug builds.
| */ | ||
| module.exports = function createStoreCDO(reducer, initialState) { | ||
| if (process.env.NODE_ENV !== "production") { | ||
| var createLogger = require('redux-logger'); |
There was a problem hiding this comment.
I prefer require's to always be at the top of the file.
There was a problem hiding this comment.
I did this one in particular inline, because we want to remove it in production builds.
There was a problem hiding this comment.
Oh, that makes sense. Could we not still have it within a conditional at the top of the file?
|
I'm pretty pumped to start using redux here :) |
| * | ||
| * @returns {{type: ActionType, props: Object}} | ||
| */ | ||
| module.exports.setLevelProps = function (props) { |
There was a problem hiding this comment.
Should we call this setInitialLevelProps to be a little more specific/clear here?
| // have levelHtml stored due to a previous bug. HTML set by levelbuilder | ||
| // is stored in startHtml, not levelHtml. | ||
| if (config.level.hideDesignMode) { | ||
| if (Applab.reduxStore.getState().isDesignModeHidden) { |
There was a problem hiding this comment.
Made a comment about this elsewhere as well, but I think it should be a goal to directly access the store as little as possible.
There was a problem hiding this comment.
Why? As long as we're not mutating it, isn't it better to access the store than to duplicate state?
I can see the argument for hiding access to the store behind helper methods where we need to do this - and it will be inevitable in some places since not all of our code can ever be absorbed into React.
I see what you mean about this as a code smell though. If I'm introducing too many of these in one PR, I may be refactoring poorly, moving the wrong state into redux.
There was a problem hiding this comment.
I think that one reason is that when we're using redux in a connected component, we know that the store and the UI will always be in sync. Store changes cause a rerender (if it changes props we care about), and our UI is updated. In cases like this, someone might perform an action that results in a change to isDesignModeHidden, but we're not guaranteed to rerender this UI.
There was a problem hiding this comment.
Hmm. Two thoughts.
- In this case we are both in the
initmethod, and the "state" we're examining is a level property that's supposed to stay the same throughout the session. For something like the level properties which are effectively a bundle of constants, would it be better to freeze it and stick it on a global than to put it in redux? - The other cases you're commented on do actually refer to changing state (
currentScreenIdin particular) and I agree that depending on the store in a non-React context takes some care. I don't think it's actively harmful though - all of our non-React view code has to decide for itself when to re-render, even before implementing redux. If anything it's easier now because we can subscribe to the store if we're worried about missing an update - something you can't do when depending on the DOM for state, as we do in some of our App Lab code.
| onModeChange(state.mode); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
One reason that having a bunch of subscribers that update UI vs. having a single root component that subscribes/connects is that it's not much less predictable what order things happen in.
|
Extracted two changes that can happen before this PR:
|
|
@Bjvanminnen I think I've hit a stopping point with this PR. Final thoughts? |
| isViewDataButtonHidden: !!config.level.hideViewDataButton | ||
| })); | ||
|
|
||
| Applab.reduxStore.dispatch(changeInterfaceMode(Applab.startInDesignMode() ? ApplabInterfaceMode.DESIGN : ApplabInterfaceMode.CODE)); |
There was a problem hiding this comment.
Good name :) ("interface mode")
|
Added a couple more minor comments, but generally lgtm :) Look forward to having redux :) :) |
React Wrapper Step 6: Reduxify App Lab
Gives App Lab a redux store and starts using it to drive rendering of the React frame.
New Redux Actions
There are only three of them in this first pass:
setInitialLevelPropsis the least exciting of the three. It gets called once during init to push a bunch of properties into the store, which makes them easy to use from any component without writing much glue code. Did cleanup as I went to ensure that the store is the single source of truth for these values after initialization.changeModeswitches between Code Mode and Design Mode, making redux the single source of truth for which mode we are currently in. This let me stop passing thestartInDesignModeproperty to React, which shouldn't care about initial state in particular. The non-React code that depends on this value now manually subscribes to redux to be notified when this value changes. I think it's centralized things nicely.changeScreenwhich changes the focused/active screen id in design mode. Similarly depends on a manual subscription to redux to respond appropriately to the change in state. Candidate future work: ReduxifyaddScreenanddeleteScreen.Next steps
DesignWorkspacea React-descendant ofAppLabViewand not needing to render it manually anymore. Unfortunately, it's rendering inside#codeWorkspacewhich is one of our ProtectedStatefulDivs because all sorts of non-React stuff hangs off of it. It's going to be tricky to do a clean separation of concerns there.Step 6 from this design document.