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

React Wrapper Step 6: Reduxify App Lab #7148

Merged
merged 36 commits into from Mar 14, 2016
Merged

React Wrapper Step 6: Reduxify App Lab #7148

merged 36 commits into from Mar 14, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Mar 4, 2016

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:

  1. setInitialLevelProps is 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.
  2. changeMode switches 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 the startInDesignMode property 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.
  3. changeScreen which 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: Reduxify addScreen and deleteScreen.

Next steps

  • We're very close to making DesignWorkspace a React-descendant of AppLabView and not needing to render it manually anymore. Unfortunately, it's rendering inside #codeWorkspace which 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.

islemaster added 14 commits Mar 4, 2016
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');

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

I prefer require's to always be at the top of the file.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 4, 2016

Author Member

I did this one in particular inline, because we want to remove it in production builds.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

Oh, that makes sense. Could we not still have it within a conditional at the top of the file?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 5, 2016

Author Member

Sure thing.

if (process.env.NODE_ENV !== "production") {
var createLogger = require('redux-logger');
var reduxLogger = createLogger();
var devTools = window.devToolsExtension ?

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

Might be worth a comment describing what the story is with window.devToolsExtension

@@ -0,0 +1,42 @@
/** @file Redux action-creators for App Lab.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

Might be too late, but my preference would be that this is lower case actions since we're not exposing a constructor/class.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 5, 2016

Author Member

Not too late - only a problem once this code hits a MacBook 😛

newState[propName] = action.props[propName];
}
});
return newState;

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

We could use _.pick and do something like:

var newState = $.extend({}, state, _.pick(action.props, ['assetUrl', ... 'isViewDataButtonHidden']))

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 5, 2016

Author Member

If I'm going to use _.pick then I might as well use _.assign too, instead of $.extend. Neither pick nor assign are currently part of our custom lodash build. Do I:

  1. Add pick and assign to our lodash build?
  2. Just add pick and use with with extend?
  3. Finally include all of lodash?

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 5, 2016

Contributor

Looks like this comment disappeared from the PR, but I would say #3. Note:
In a world with ES6, we can just do:
var newState = { ...state, _.pick() };

I wonder if really what we want instead of picking off valid properties is
to look for props that aren't in our list, and then throw if we see them.
At that point, we can just do:
var newState = $.extend({}, state, action.props)

On Fri, Mar 4, 2016 at 4:28 PM, Brad Buchanan notifications@github.com
wrote:

In apps/src/applab/Reducers.js
#7148 (comment)
:

  • switch (action.type) {
  • case ActionType.SET_LEVEL_PROPS:
  •  var newState = $.extend({}, state);
    
  •  [
    
  •    'assetUrl',
    
  •    'isDesignModeHidden',
    
  •    'isEmbedView',
    
  •    'isReadOnlyWorkspace',
    
  •    'isShareView',
    
  •    'isViewDataButtonHidden'
    
  •  ].forEach(function (propName) {
    
  •    if (typeof action.props[propName] !== 'undefined') {
    
  •      newState[propName] = action.props[propName];
    
  •    }
    
  •  });
    
  •  return newState;
    

If I'm going to use _.pick then I might as well use _.assign too, instead
of $.extend. Neither pick nor assign are currently part of our custom
lodash build. Do I:

  1. Add pick and assign to our lodash build?
  2. Just add pick and use with with extend?
  3. Finally include all of lodash?


Reply to this email directly or view it on GitHub
https://github.com/code-dot-org/code-dot-org/pull/7148/files#r55108757.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 5, 2016

Author Member

Actually, I like your alternate suggestion. Updated.

state = {
assetUrl: function () {}
};
}

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

A common form I see for reducer's is something like this (ES6):

const initialState = {
  assetUrl: function () {}
};

function rootReducer(state = initialState, action) {
 ...
}

We could accomplish similar in ES5 by doing this:

var initialState = {
  // we could also initialize all of our other props to null/undefined here so that we have
  // an easy way to see the shape of  our state
  assetUrl: function () {}
};

function rootReducer(state, action) {
  state = state || initialState; // we don't have to worry about valid falsey states
 ...
}
}
});
module.exports = AppLabView;
module.exports = connect(
(state) => ({

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 4, 2016

Contributor

We're not really supposed to be using ES6 yet...

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 5, 2016

Author Member

😝 Fine. Thought I might get away with it in JSX...

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 5, 2016

Contributor

Though this is a .jsx file, what you're writing here is still just Javascript :-P

It does seem like there's some silliness in not allowing it, when it works just fine, but I do think there's value in having an explicit point when we start using it (and have good linting, etc. around it). I do wonder some though if not getting to use ES6 will make redux feel somewhat less pleasant :(

isEmbedView: state.isEmbedView,
isShareView: state.isShareView
})
)(StudioAppWrapper);

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 5, 2016

Contributor

I don't love the idea of having two different components, one of which is connected and one of which isnt. Do we expect this to just be necessary for transitional purposes, or a more long term approach?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 5, 2016

Author Member

This might be fairly long-term. I don't forsee a use for Redux in our "older" apps in the near future, and this happens to be the root of several components currently shared between them and App Lab.

I don't see having a separate 'connected' version as an issue, especially given that it's the recommended react-redux approach. Do you expect this to be confusing?

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 5, 2016

Contributor

I suppose not, given that it's got a pretty clearly distinct name. I guess I'm fine with this, as I don't really have much justification for my original distaste.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 5, 2016

I'm pretty pumped to start using redux here :)

*
* @returns {{type: ActionType, props: Object}}
*/
module.exports.setLevelProps = function (props) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

Should we call this setInitialLevelProps to be a little more specific/clear here?

* @param {!ApplabMode} mode
* @returns {{type: ActionType, mode: ApplabMode}}
*/
module.exports.changeMode = function (mode) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

I wonder if we can come up with a name that's a little more specific? We have a lot of different modes (i.e. design/code, text/block, probably others).

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

I'm open to suggestions. I renamed the enum ApplabMode but even that isn't particularly specific. EditorMode or InterfaceMode aren't sufficient to distinguish code/design vs block/text.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

One option would be to make this toggleDesignMode(on | off). I'm not sure I like that better.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

Could do that for now. I was hoping to be forward-thinking to the including of a third mode (for makerlab)

@@ -1,29 +1,27 @@
/** @file Top-level view for App Lab */
// Strict linting: Absorb into global config when possible
/* jshint

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

jshint wont accomplish anything now fyi

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

Thanks. I need to go through and remove this stuff.

onViewDataButton: React.PropTypes.func.isRequired,
onScreenChange: React.PropTypes.func.isRequired,

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

so much red :)

@@ -136,6 +135,10 @@ VisualizationOverlay.prototype.onSvgMouseMove_ = function (event) {
this.mousePos_ = this.mousePos_.matrixTransform(this.screenSpaceToAppSpaceTransform_);
}

if (this.shouldShowCrosshair_()) {
this.mouseoverApplabControlId_ = this.getMouseoverApplabControlId_(event.target);
}

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

Could extract from this PR: This fixes an uncaught exception I encountered in testing where the overlay where ti doesn't get the element ID correctly when running (where we're not going to display the overlay anyway).

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

I dont care too much, but that is generally preferable.

}
});
module.exports = AppLabView;
module.exports = connect(function propsFromState(state) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

What do you think about calling this propsFromStore or even just selector(probably less good)? I worry a little about confusing this "state" with component state.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

propsFromStore sounds good. Could also leave this anonymous, but it seemed nice to name these.

onScreenCreate: designMode.createScreen
});
ReactDOM.render(React.createElement(AppLabView, nextProps), Applab.reactMountPoint_);
/* jshint ignore:start */

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

Note to self: Remove jshint block.

case ActionType.CHANGE_SCREEN:
if (state.currentScreenId === action.screenId) {
return state;
}

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

I'm in progress on breaking this up, which may be overkill but i feel like modeling it now will demonstrate how to scale it later.

designMode.editElementProperties(elementUtils.getPrefixedElementById(currentScreenId));
designMode.editElementProperties(
elementUtils.getPrefixedElementById(
Applab.reduxStore.getState().currentScreenId));

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

I think we want to limit how much we do stuff like this (accessing the store directly). Ideally we wouldn't at all, but that might be difficult in a world where not all of our UI is react.

@@ -680,7 +694,7 @@ Applab.init = function(config) {
// should never be present on such levels, however some levels do
// 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) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

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.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

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.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

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.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 12, 2016

Author Member

Hmm. Two thoughts.

  • In this case we are both in the init method, 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 (currentScreenId in 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);
}
});
};

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 12, 2016

Contributor

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.

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Mar 14, 2016

Extracted two changes that can happen before this PR:

  • #7276 fixing the bug in the element id overlay
  • #7277 removing the now-useless jshint annotations from apps.
@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Mar 14, 2016

@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));

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 14, 2016

Contributor

Good name :) ("interface mode")

* Subscribe to state changes on the store.
* @param {!Store} store
*/
function subscribeToRedux(store) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 14, 2016

Contributor

Based on the name, I was initially expecting this to be a method I call when I want to set up a new subscriber. In fact, this is a method that sets up some subscribers for us, and should never be called more than once. What about something like setupReduxSubscribers or configureReduxSubscribers (or maybe subscriptions instead of subscribers?)

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 14, 2016

Contributor

would probably then also rename the designMode version that we call.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 14, 2016

Author Member

Good call, I'll do that.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 14, 2016

Added a couple more minor comments, but generally lgtm :) Look forward to having redux :) :)

islemaster added a commit that referenced this pull request Mar 14, 2016
React Wrapper Step 6: Reduxify App Lab
@islemaster islemaster merged commit 120208e into staging Mar 14, 2016
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on reduxify-applab at 85.19%
Details
hound No violations found. Woof!
@islemaster islemaster deleted the reduxify-applab branch Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.