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

Add code splitting support to RootElement #963

Merged
merged 3 commits into from
Oct 18, 2017
Merged

Add code splitting support to RootElement #963

merged 3 commits into from
Oct 18, 2017

Conversation

davidk107
Copy link
Contributor

This introduces a componentLoader and childProps prop to RootElement. componentLoader takes in an asynchronous function that resolves to a component. RootElement will pause on rendering until componentLoader has been resolved. Using this with dynamic code loaders such as require.ensure or import() can result in having server side rendered sections without having to download the client side Javascript until later in the page loading process.

childProps will be additional props that can be passed to the resolved component. childProps are mainly used for sending down synchronous props. It's usage is not restricted to componentLoader and can be used with out it.

Examples and more detailed descriptions can be found under https://github.com/davidk107/react-server/tree/add-componentLoader-support/packages/react-server-examples/code-splitting

@@ -29,8 +36,10 @@ class RootElement extends React.Component {

// Okay, now we've complained about it
// sufficiently, let's go ahead and update.
this.props = _.assign({}, this.props, {childProps});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look into React internals and it appears that setting this.props = ... has no effect whatsoever. Thus the subsequent call to this.forceUpdate() re-renders the component without any prop updates. By using setState this will also fix the issue described in #884

// if we have a component loader specified, copy the resolved component
// and render that with the current child as a child of that component
const currentChild = rendered.props.children;
const childToRender = componentLoaderDeferred ? React.createElement(loadedComponent.value, null, currentChild) : currentChild;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this will always create a new element from the componentLoaderDeferred if there was one.

Should this only use loadedComponent.value on the first pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. What do we mean by on the first pass?

var [whenResult, loadedComponent] = results;
if (whenResult.value || loadedComponent.value || childProps) {
// merge in child props from listen, when, and childProps
const clonedChildProps = _.assign({}, rendered.props.childProps, whenResult.value, childProps);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to also include rendered.state.childProps now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it out and it seems that at this point the constructor for RootElement has not been called (which is to be expected) Thus rendered.state.childProps will be undefined. However even if the constructor is called, both rendered.state.childProps and rendered.props.childProps will be the same object and so we are fine with just relying on using rendered.props.childProps here.

I can say that I am confident that rendered's componentDidMount will not have been called. That is what hooks up the listener to listen which might cause changes to rendered.state.childProps.

"clean": "rm -rf build __clientTemp",
"test": "echo \"Error: no test specified\" && exit 1"
},
"author": "Sasha Aickin",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be you. :)

// Incase we receive new props such as during client transitions
// We will want to update our state's childProp with any new childProps
// that may have been passed in. This still respects props as the ultimate source of truth
const newChildProps = _.assign({}, this.state.childProps, nextProps.childProps);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside to doing this is that if nextProps.childProps is empty, then newChildProps will still retain old keys/values from this.state.childProps. The workaround to this would be to pass in null as the new value for those keys if that behavior is desired.

@gigabo gigabo added the enhancement New functionality. label Oct 18, 2017
@gigabo gigabo merged commit 7f7f16f into redfin:master Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants