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 1: Simple React Wrapper that never updates #7000

Merged
merged 17 commits into from Feb 26, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Feb 26, 2016

Introduces two new React components, and uses them in all app types except for NetSim (which has its own top-level view).

  • AppView is an extremely simple wrapper component, designed to mount to #codeApp and hold everything currently rendered by page.html.ejs. It has two required properties:
    • renderCodeApp should be a function that returns the rendered HTML from page.html.ejs (formerly the complete contents of #codeApp).
    • onMount is a callback that gets called when the AppView and everything underneath it is rendered, mounted, and ready to use.
  • ProtectedStatefulDiv renders as an ordinary <div>, but it will never update its contents (via React) and it will throw an exception if it's ever unmounted. It's designed to protect sections of the DOM that may be stateful (like #designModeViz) or should be controlled by our own non-React code or by another library (like droplet or blockly).

Each app's init function has been modified to render AppView, with the page.html.ejs rendering happening inside renderCodeApp, and studioApp.init() and everything following happening inside the onMount callback. Pseudocode:

// Before
App.init = function (config) {
  // pre-init work
  config.html = page(options);
  studioApp.init(config);
  // post-init work
};

// After
App.init = function (config) {
  // pre-init work
  React.render(React.createElement(AppView, {
    renderCodeApp: function () {
      return page(options);
    },
    onMount: function () {
      studioApp.init(config);
      // post-init work
    }
  }), document.getElementById(config.containerId));
};

It's less concise for now, but this is the first in a series of changes that make it easier to extend our apps with React. See this design document for more details.

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Feb 26, 2016

Note: I strongly recommend viewing this diff ignoring whitespace changes. It makes it a lot more obvious what transformations are going on.

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Feb 26, 2016


componentWillUnmount: function () {
throw new Error("Unmounting a ProtectedStatefulDiv is not allowed.");
},

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

This error might be thrown when we navigate away from the page, or there might be another situation when we want to safely unmount the parent react component. How about adding a required okToUnmount prop, which the parent sets to true via its own state, in its own componentWillUnmount call?

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 26, 2016

Author Member

I thought about that, and have verified that the component does not unmount when navigating away from the page.

I'm tempted to hold off on writing the 'okToUnmount' path until it's actually needed. Thoughts?

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

Sounds good -- let's hold off on okToUnmount for now.

// Use offsetWidth of viz so we can include any possible border width:
vizCol.style.maxWidth = viz.offsetWidth + 'px';
}
React.render(React.createElement(AppView, {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 26, 2016

Contributor

I think this is a great way of transitioning from our current world to our new world. However, long term I think we want to discourage having this much content in our createElement call (to some extent referring to number of properties, but mostly referring to having full function definitions).

If it's not difficult, I'd love to get to the place in the nearish future where we can have JSX instead of createElement which will discourage huge objects like this.

In any case, no changes are needed in this regards for this PR

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 26, 2016

Author Member

I'd be happy to go through and extract the parameters into local variables, to make the expression of these props as compact as possible. That said, I think some of that will be mitigated as we get further into this process and break up page.html.ejs into smaller pieces that need fewer parameters.

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

@Bjvanminnen would you like these better if the functions were simply defined outside the React.createElement call, then passed in by name?

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 26, 2016

Contributor

Yep, I would like that better. Though again, I think this is a fine first step (and results in a more minimal change with whitespace ignored). Moving out these functions might be a good step for a future PR.

loadAudio: function () {
React.render(React.createElement(AppView, {
renderCodeApp: function () {
return require('../templates/page.html.ejs')({

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 26, 2016

Contributor

I don't like requires in the middle of the file like this. I realize this isnt new, so your discretion as to whether you want to change or not.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Feb 26, 2016

General note: It might be worth learning about/understanding a concept called "Portals" in React. I haven't fully researched this myself, but I believe it's a way to restrict React's influence to subsets of your React tree (and allow portions in the middle to be modified by non-React code). Might be interesting for some of these use cases (also possible I've somewhat misunderstood the concept).

return <ProtectedStatefulDiv ref="codeApp" />;
}
});
module.exports = AppView;

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 26, 2016

Contributor

Cool 👍

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Feb 26, 2016

generally lgtm. excited to increase the scope of react in our projects :)


render: function () {
return <ProtectedStatefulDiv ref="codeApp" />;
}

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

Awesome component! I don't like that the parent is rendering the child's contents, however this does have the benefit of ensuring that the required child component does get rendered. If this is intentional, let's have a comment explaining that we're doing it this way to ensure the required component is rendered.

Alternately, the rendering method could be passed to the ProtectedStatefulDiv which would then render itself in its own componentDidMount method, and we'd have an assertion or something here to ensure that all required components (or DOM element IDs) exist in the DOM. However this is unreliable because you can forget to assert.

The scenario here is that e.g. we always render code mode (visible) and design mode (hidden), but on levels when we show design mode first we might forget to initially render code mode. This would be nice to catch at render time (just as it would be nice to catch a required component unmounting at the time it is unmounted as ProtectedStatefulDiv currently does).

Bear with me for some brainstorming that may nor may not reflect something we want to spend time on now:

In an amazing world, we could use a pattern which knows that component A has a required, protected child component B (or possibly required DOM element id #b). I first thought of mixins, but apparently mixins are dead in favor of composition: https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750#.kp8veb9xf Following the example laid out in the article, you could have a function which says "wrap Component A by making sure it (initially and always) has a child DOM node with id #b":

var AppView = React.createClass({
  ...
  render: function () {
    return <div id="codeApp" />
  }
});
AppView = requiredChildren(AppView, ['codeApp']);
function requiredChildren(Component, children) {
  // Magic trick to make sure child component is never unmounted, this part may be hard
}

Or something like that. To reiterate, I don't think this is strictly necessary but could be a useful if we think we'll use this pattern a lot.

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 26, 2016

Author Member

Oh yeah, not sure why I didn't just pass the render method as a property to the ProtectedStatefulDiv. That's a good idea.

I'm not sure I 100% understand the scenario and additional validation you're describing, so let me echo it to see if we're on the same page.

You're saying the ProtectedStatefulDiv guards against unmounting but not against forgetting to mount it in the first place? That's true, but in practice I don't know how problematic it is.

Let's consider a your code mode/design mode example. Both modes contain a ProtectedStatefulDiv at some level (code mode because of droplet, design mode because of the stateful work area.

To write your top-level view to safely allow switching between the two, it must always render both (using style to hide one or the other).

render: function () {
  var show = {display: 'block'};
  var hide = {display: 'none'};
  var codeStyle = this.props.mode === 'CODE' ? show : hide;
  var designStyle = this.props.mode === 'DESIGN' ? show : hide;
  return <div>
    <CodeMode style={codeStyle} />
    <DesignMode style={designStyle} />
  </div>;
}

Using this model, you'll never fail to render either, even on initial render, regardless of whether you load in code mode or design mode.

I imagine you're picturing a scenario more like this:

render: function () {
  if (this.props.mode === 'CODE') {
    return <div><CodeMode /></div>;
  } else if (this.props.mode === 'DESIGN') {
    return <div><DesignMode /></div>;
  }
  return <div></div>;
}

But that model, while it would succeed on initial render (ignoring the fact that studioApp.init would blow up because it depends on all sorts of stuff existing after that initial render is done) will throw as soon as you switch modes because it would unmount one component or the other. So I'm having trouble thinking of a practical scenario in which this is likely to go unnoticed.

Are you wanting the top-level view to somehow also assert that all of its descendant ProtectedStatefulDivs have been rendered when it's initially mounted? You could give it special knowledge about all the stateful areas in its descendant components so it can do that validation. The downside to this is that the dev has to remember to add that validation any time they introduce a new stateful area, so you're equally vulnerable to developer error as we are now - I'm not sure it adds much protection over what we've got now.

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

Brad, your understanding of the scenario I was describing is correct. I also think you are right that the problem of forgetting to unmount the div is unlikely to go unnoticed. The related problem of accidentally re-rendering the div is much more likely to go unnoticed and cause weird bugs. ProtectedStatefulDiv sounds like the way to go.

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 26, 2016

Author Member

I'm not saying the validation is a bad idea, I just can't think of a way to make it automatic enough to catch all cases. Here's some brainstorming:

  • The top-level view component could wrap all of its ejs rendering functions and verify that they've been called exactly once in componentDidMount. That still requires the dev to wrap any new ejs rendering function they add to the top-level propTypes, but that seems less likely to get missed.
  • Instead of wrapping those functions, this concept might be easy to implement as a unit test, passing in fake render functions that sense whether they've been called, then rendering the view with as many property permutations as possible. But again, vulnerable to developer error.
  • I wonder if there's some magic way to make a component report 'hasStatefulChildren' if it requires ProtectedStatefulDiv.jsx or any other component that reports hasStatefulChildren is true. Then you could do some validation that any component that requires a stateful child renders at least one. That doesn't help if a component has multiple stateful children though.

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

One problem with the hasStatefulChildren approach is that if you forget to render the child which is a ProtectedStatefulDiv, then the component won't have stateful children :/ I see now that the wrapping solution I proposed has the problem that a developer can still forget to wrap their component.

I'm open to brainstorming further, but your investigation of the code/design example leads me to believe that we're likely to catch the problem of forgetting to initially render a stateful child in an understandable way, so I'm inclined to not to block merging of this PR on further brainstorming.

container.innerHTML = config.html;
if (typeof config.html !== 'undefined') {
container.innerHTML = config.html;
}

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

Why is it safe to silently ignore an undefined config.html?

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

Oh I see, config.html comes from the server and can be empty. 👍

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 26, 2016

Author Member

When we do our top-level rendering with React (prior to StudioApp.init) we want to skip this step, which would clobber everything React just rendered.

In my prototype I originally skipped this with a more explicit config variable:

if (!config.usingReactRendering) {
  container.innerHTML = config.html;
}

That felt a little like duplicating state though - why even assign config.html if we're not going to use it, and then why introduce another config variable to say we're not using config.html when we can just check if it's empty?

Alternatively, I could tear out this line altogether if I also give NetSim a top-level React wrapper (it's the one remaining user of this old approach).

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Feb 26, 2016

Contributor

Thanks for explaining, it was not clear that this conditional is based on whether this app is using react to render. I think it would suffice to add a comment noting that config.html will be empty for apps which are rendering via React.

@davidsbailey

This comment has been minimized.

Copy link
Contributor

davidsbailey commented Feb 26, 2016

LGTM after considering comments

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Feb 26, 2016

@Bjvanminnen

General note: It might be worth learning about/understanding a concept called "Portals" in React. I haven't fully researched this myself, but I believe it's a way to restrict React's influence to subsets of your React tree (and allow portions in the middle to be modified by non-React code). Might be interesting for some of these use cases (also possible I've somewhat misunderstood the concept).

I think you've got it backwards. Here's what I could quickly dig up on "React Portals":

Pretty sure these articles are referring to a method for extending React rendering to non-React parts of your DOM, as opposed to restricting React's influence within React-owned parts of the DOM.

islemaster added a commit that referenced this pull request Feb 26, 2016
React Wrapper Step 1: Simple React Wrapper that never updates
@islemaster islemaster merged commit cb90855 into staging Feb 26, 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 Coverage decreased (-0.0%) to 85.763%
Details
hound No violations found. Woof!
@islemaster islemaster deleted the react-wrapper branch Feb 26, 2016
deploy-code-org added a commit that referenced this pull request Feb 26, 2016
cb90855 Merge pull request #7000 from code-dot-org/react-wrapper (Brad Buchanan)
bdc9677 Merge pull request #6995 from code-dot-org/auto-toolbox (Ram Kandasamy)
a5d5cde Merge pull request #7016 from code-dot-org/applabShareImage (Brent Van Minnen)
0318de0 ProtectedStatefulDiv renders own contents (Brad Buchanan)
46a9f2b Move page.html.ejs require in craft.js to top of file (Brad Buchanan)
e142f4f Comment on skipping HTML injection (Brad Buchanan)
58bf5bf better share image for applab (Brent Van Minnen)
c62f46e Merge pull request #6970 from code-dot-org/cdo-nginx (Will Jordan)
e89087e Add nginx to application infrastructure (currently disabled by default; enable with node['cdo-apps']['nginx_enabled'] = true) (Will Jordan)
658ad08 Merge pull request #7013 from code-dot-org/levelbuilder (Mehal Shah)
bd6e1e1 Responding to PR comments (Ram Kandasamy)
65c6903 Merge pull request #7010 from code-dot-org/pegasus_test_fix (Brent Van Minnen)
00cb224 Merge pull request #7012 from code-dot-org/enable_activity_monitor (philbogle)
af9183d levelbuilder content changes (-Mehal) (Continuous Integration)
6819aec staging content changes (-Mehal) (Continuous Integration)
153ba6e Merge branch 'staging' of github.com:code-dot-org/code-dot-org into staging (Continuous Integration)
bead5ab staging content changes (-Mehal) (Continuous Integration)
ae184c5 Merge pull request #7006 from code-dot-org/performanceRedundantMerge (ashercodeorg)
@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented Feb 26, 2016

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.