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.

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

Allow RootElements and RootContainers to specify their node type #532

Closed
wants to merge 9 commits into from
Closed

Allow RootElements and RootContainers to specify their node type #532

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c5202aa
tagNames on root containers now working
Aug 11, 2016
0fbc64b
tagNames on root containers now working
Aug 11, 2016
3099089
Merge branch 'root-node-tagnames' of https://github.com/kcjonson/reac…
Aug 11, 2016
9a6d065
updates to attributes test page
Aug 13, 2016
48f83f4
Merge branch 'master' of https://github.com/kcjonson/react-server int…
Aug 13, 2016
99b7227
Revert "updates to attributes test page"
Aug 13, 2016
b09a5dc
updates to attributes test page
Aug 13, 2016
8d362c1
moving tagName out of attrs
Aug 13, 2016
0d91e5c
further work, but not finished yet
Aug 13, 2016
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
16 changes: 10 additions & 6 deletions packages/react-server/core/ClientController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,10 +580,10 @@ class ClientController extends EventEmitter {
//
if (this._reuseDom) {
oldRootElement = document.querySelector(
`div[${REACT_SERVER_DATA_ATTRIBUTE}="${index}"]`
`*[${REACT_SERVER_DATA_ATTRIBUTE}="${index}"]`
);
oldRootContainer = document.querySelector(
`div[${PAGE_CONTAINER_NODE_ID}="${index}"]`
`*[${PAGE_CONTAINER_NODE_ID}="${index}"]`
);
}

Expand Down Expand Up @@ -650,7 +650,11 @@ class ClientController extends EventEmitter {

_.forEach(
getRootElementAttributes(element),
(v, k) => root.setAttribute(k, v)
(v, k) => {
if (k !== 'tagName') {
root.setAttribute(k, v)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit actually isn't necessary. The attributes we pass through are white listed in RootElement.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I hadn't gotten far enough... looks like it's being added as an attr there now. Hmm...

Copy link
Author

@kcjonson kcjonson Aug 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yea, its a kinda ugly bit of code. No harm arises by putting the attribute on the node, but its redundant information because the node with be of the type specified so I chose to exclude it.

);

totalRenderTime += timer.stop();
Expand Down Expand Up @@ -724,7 +728,7 @@ class ClientController extends EventEmitter {
logger.debug("Removing previous page's React components");

[].slice.call(
document.querySelectorAll(`div[${REACT_SERVER_DATA_ATTRIBUTE}]`)
document.querySelectorAll(`*[${REACT_SERVER_DATA_ATTRIBUTE}]`)
).forEach((root, i) => {
if (i >= index) {
// Since this node has a "data-react-server-root-id"
Expand All @@ -738,7 +742,7 @@ class ClientController extends EventEmitter {
});

[].slice.call(
document.querySelectorAll(`div[${PAGE_CONTAINER_NODE_ID}]`)
document.querySelectorAll(`*[${PAGE_CONTAINER_NODE_ID}]`)
).forEach((root, i) => {
if (i >= index) {
// Gotta get rid of our containers,
Expand Down Expand Up @@ -860,7 +864,7 @@ class ClientController extends EventEmitter {
for (var i = startIndex; i <= endIndex; i++) {
this._ensureRootNodeDfd(i).resolve(
this.mountNode.querySelector(
`div[${REACT_SERVER_DATA_ATTRIBUTE}="${i}"]`
`*[${REACT_SERVER_DATA_ATTRIBUTE}="${i}"]`
)
);
}
Expand Down
6 changes: 4 additions & 2 deletions packages/react-server/core/components/RootContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ RootContainer.defaultProps = {
}

RootContainer.flattenForRender = function(element) {
return [{containerOpen: getRootElementAttributes(element)}]
let attrs = getRootElementAttributes(element) || {};
attrs.tagName = attrs.tagName || 'div';
return [{containerOpen: true, attrs: attrs}]
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like {containerOpen: tagName, attrs} would be nicer here, to keep attrs and tagName separate.

.concat(prepChildren(element))
.concat([{containerClose: true}])
.concat([{containerClose: true, attrs: attrs}])
Copy link
Contributor

Choose a reason for hiding this comment

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

Then this could just be {containerClose: tagName}, and the (irrelevant for a close tag) attrs can be omitted.

.reduce((m,v) => m.concat(Array.isArray(v)?v:[v]), [])
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react-server/core/components/RootElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ RootElement.getRootElementAttributes = function(element) {
'id',
'style',
].forEach(k => props[k] && (attrs[k] = props[k]));

attrs.tagName = props.tagName || 'div';
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't bundle this in with attrs. That's really meant to just be HTML attributes.

The tag name is a different beast.

Copy link
Author

Choose a reason for hiding this comment

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

Yea, I was waffling a bit on that one, flipped a coin and came out this way. I think I agree with you that the other way is easer to read.

return attrs;
}

Expand Down
18 changes: 13 additions & 5 deletions packages/react-server/core/renderMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,26 +866,34 @@ function writeElement(res, element, i){
html : '',
}
}

let tagName = 'div';
if (element.attrs.tagName) {
tagName = element.attrs.tagName;
delete element.attrs.tagName;
}

if (element.containerOpen) {
res.write(`<div ${PAGE_CONTAINER_NODE_ID}=${i}${
_.map(element.containerOpen, (v, k) => ` ${k}="${attrfy(v)}"`)
res.write(`<${tagName} ${PAGE_CONTAINER_NODE_ID}=${i}${
_.map(element.attrs, (v, k) => ` ${k}="${attrfy(v)}"`)
}>`);
} else if (element.containerClose) {
res.write('</div>');
res.write(`</${tagName}>`);
} else if (element.isTheFold) {

// Okay, we've sent all of our above-the-fold HTML,
// now we can let the client start waking nodes up.
bootstrapClient(res, i)
} else {
res.write(`<div data-react-server-root-id=${

res.write(`<${tagName} data-react-server-root-id=${
i
} data-react-server-timing-offset="${
// Mark when we sent it.
new Date - RLS().timingDataT0
}"${
_.map(element.attrs, (v, k) => ` ${k}="${attrfy(v)}"`)
}>${element.html}</div>`);
}>${element.html}</${tagName}>`);
}
}

Expand Down