Skip to content

add render component extension point #1064

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

add render component extension point #1064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ee3557f
add renderComponent extension point
edmond-c Apr 25, 2020
7cb3d15
doc + default
edmond-c Apr 25, 2020
acdee60
mirror getConstructor structure
edmond-c Apr 25, 2020
6543967
add missing variable
edmond-c Apr 25, 2020
bfe0549
update readme
edmond-c Apr 25, 2020
ad27aee
rollback hot reload specfic diffs
edmond-c Apr 25, 2020
fffc5e8
Update index.js
EdmondChuiHW Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions react_ujs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ var detectEvents = require("./src/events/detect")
var constructorFromGlobal = require("./src/getConstructor/fromGlobal")
var constructorFromRequireContextWithGlobalFallback = require("./src/getConstructor/fromRequireContextWithGlobalFallback")

var renderWithReactDOM = require("./src/renderComponent/withReactDOM")
Copy link
Author

@EdmondChuiHW EdmondChuiHW Apr 26, 2020

Choose a reason for hiding this comment

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

folder structure and import pattern is taking reference from getConstructor for consistency


var ReactRailsUJS = {
// This attribute holds the name of component which should be mounted
// example: `data-react-class="MyApp.Items.EditForm"`
Expand Down Expand Up @@ -71,6 +73,11 @@ var ReactRailsUJS = {
useContext: function(requireContext) {
this.getConstructor = constructorFromRequireContextWithGlobalFallback(requireContext)
},

// Called after React unmounts component at `node`
// Override this function to perform any cleanup
// the default function does nothing
onComponentUnmountAtNode: function (node) {},

// Render `componentName` with `props` to a string,
// using the specified `renderFunction` from `react-dom/server`.
Expand All @@ -80,6 +87,11 @@ var ReactRailsUJS = {
return ReactDOMServer[renderFunction](element)
},

// Render `component` using the specified `renderFunction` from `react-dom`.
// Override this function to render components in a custom way.
// function signature: ("hydrate" | "render", component, node, props)
renderComponent: renderWithReactDOM,
Comment on lines +90 to +93
Copy link
Author

@EdmondChuiHW EdmondChuiHW Apr 26, 2020

Choose a reason for hiding this comment

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

The comment and pattern here intends to mirror getConstructor. Let me know if

  1. the function signature can be better represented
  2. if the type of renderFunctionName ("hydrate" | "render") should be made more generic, i.e. string
  3. export the renderWithReactDOM function or assign it in a constant, e.g. ReactRailsUJS.DEFAULT_RENDER_COMPONENT_FUNCTION

Reference:

react-rails/react_ujs/index.js

Lines 63 to 66 in d5da111

// Get the constructor for a className (returns a React class)
// Override this function to lookup classes in a custom way,
// the default is ReactRailsUJS.ComponentGlobal
getConstructor: constructorFromGlobal,


// Within `searchSelector`, find nodes which should have React components
// inside them, and mount them with their props.
mountComponents: function(searchSelector) {
Expand Down Expand Up @@ -112,9 +124,9 @@ var ReactRailsUJS = {
}

if (hydrate && typeof ReactDOM.hydrate === "function") {
component = ReactDOM.hydrate(component, node);
renderComponent("hydrate", component, node, props);
} else {
component = ReactDOM.render(component, node);
renderComponent("render", component, node, props);
}
Comment on lines 126 to 130
Copy link
Author

@EdmondChuiHW EdmondChuiHW Apr 26, 2020

Choose a reason for hiding this comment

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

Assignment to component is removed here. It appears unused, since the function returns immediately afterwards. In addition, the return value from ReactDOM.render has been marked as deprecated.

ReactDOM.render() currently returns a reference to the root ReactComponent instance. However, using this return value is legacy and should be avoided because future versions of React may render components asynchronously in some cases. If you need a reference to the root ReactComponent instance, the preferred solution is to attach a callback ref to the root element.

}
}
Expand All @@ -128,6 +140,7 @@ var ReactRailsUJS = {
for (var i = 0; i < nodes.length; ++i) {
var node = nodes[i];
ReactDOM.unmountComponentAtNode(node);
ReactRailsUJS.onComponentUnmountAtNode(node);
}
},

Expand Down
10 changes: 10 additions & 0 deletions react_ujs/src/renderComponent/withReactDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
var ReactDOM = require("react-dom")

// Render React component via ReactDOM, for example:
//
// - `renderComponent("hydrate", component, node, props)` -> `ReactDOM.hydrate(component, node);`
// - `renderComponent("render", component, node, props)` -> `ReactDOM.render(component, node);`
//
module.exports = function(renderFunctionName, component, node) {
ReactDOM[renderFunctionName](component, node);
};
Comment on lines +3 to +10
Copy link
Author

@EdmondChuiHW EdmondChuiHW Apr 26, 2020

Choose a reason for hiding this comment

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

The last argument props here is unused in this default render function. However, it is provided to support extensibility for custom implementations. The hot reload variation, for uses props to re-render with a freshly-reloaded component constructor.

Let me know if

  1. function should declare the props argument even if it's unused (for self-documentation/ease of copy-and-pasting)
  2. function signature should change to pass constructor instead of/in addition to component. The existing implementation of cached turbolinks components would need extra considerations if component is not passed to this function

Previous proposal referenced: #595 (comment)

  • from
ReactDOM.render(React.createElement(constructor, props), node);
  • to
ReactRailsUJS.renderComponent(node, constructor, props)

// .... then ...
renderComponent: function(node, constructor, props) {
  ReactDOM.render(React.createElement(constructor, props), node);
},

Copy link
Author

@EdmondChuiHW EdmondChuiHW Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why props is required where component is already passed in? Hot reload requires a "fresh" constructor and component while re-rendering. props is required to be passed onto the "fresh" constructor. #1065 (comment)

Copy link
Author

@EdmondChuiHW EdmondChuiHW Apr 26, 2020

Choose a reason for hiding this comment

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

Considerations for removing props from the render function: while it is trivial to obtain props via node#getAttribute and JSON.prase inside this render function, the work is already done in mountComponents.

Furthermore, existing implementation of turbolinks would likely be required to move here in such case; this increases the complexity to override the render function. The intention is to keep the render function as simple as possible such that it has minimal impact to existing features when it's overriden

I do not have benchmarks/metrics to support a performance argument