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

Enable rendering into shadow roots #1167

Closed
wants to merge 2 commits into from

Conversation

BinaryMuse
Copy link

This adds the shadow root nodeType to the list of node types that can be rendered into.

I have completed the CLA.

Please let me know if I left anything out!

@martinandert
Copy link
Contributor

Would you mind explaining what this is good for?

@@ -35,6 +35,7 @@ var nodeCache = {};

var ELEMENT_NODE_TYPE = 1;
var DOC_NODE_TYPE = 9;
var SHADOW_ROOT_NODE_TYPE = 11;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you are just following what's already here, but I think we should consolidate these constants

@petehunt
Copy link
Contributor

Does this work correctly with event handling?

@BinaryMuse
Copy link
Author

@martinandert I'm specifically using it with x-tags, (see https://github.com/BinaryMuse/x-tag-react-hello-world), but more generally I think it makes sense to allow rendering into any node (that actually works as a mount point).

@petehunt Yes; I've added a click handler to the demo at https://github.com/BinaryMuse/x-tag-react-hello-world and it seems to work fine. I'd be happy to perform some additional testing along these lines.

Additionally, I'd be happy to extract those constants; I'll look at this sometime today.

@plievone
Copy link
Contributor

@BinaryMuse could you check #859 and #840 too, if your pull request addresses those concerns?

* @internal
*/
var DOMNodeTypes = {
ELEMENT: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you grep nodeType through React codebase, there are a couple of other places where for example TEXT_NODE is hardcoded as 3, don't know if those should be included too (some of those may be vendored in) or perhaps in another pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use Node.ELEMENT_NODE and the likes? Probably all the places where you want to check for nodeType already access DOM so it should be safe? See https://github.com/facebook/react/pull/423/files#r7112182 for the previous discussion on the matter

Copy link
Contributor

Choose a reason for hiding this comment

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

@andreypopp Not all browsers we support has Node.ELEMENT_NODE, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@syranide right... IE8...

Copy link
Contributor

Choose a reason for hiding this comment

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

@andreypopp Related: #893

@sebmarkbage
Copy link
Collaborator

I'm (irrationally) scared of these enum modules because I'm never sure if they'll get inlined or not. If they're not inlined, then they might not minify well and might also be a perf cost. It's probably irrational but let's just keep it as constants in the individual modules for now. The reason for having it separate is usually because it might change, but these won't. :)

I'm not quite sure what the expected behavior is for the events. Can we add a test that asserts the expected behavior? If it's not possible to test in JSDOM at the moment, it's fine to have a disabled test for now.

@sophiebits
Copy link
Collaborator

This looks stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.