React: Warn when React DOM modified by not-React

[jimfb]

As discussed in https://github.com/facebook/react/issues/3207#issuecomment-75181038, a complementary improvement is to turn on a mutation observer in dev, and warn of someone other than React modifies the DOM and/or if dom nodes appear without a data-reactid, thus indicating an extension or some other script is doing fancy magic.

[probablyup]

Seems sensible. There are some cases where outside manipulation of React-rendered DOM is desired though, like an RTE. It's safer to just set shouldComponentUpdate to false and do modifications with event listeners so the user's content doesn't accidentally get overwritten by the diff resolution.

[Daniel15]

+1. I was going to implement this but didn't have any time to do it. At work I've encountered some bugs where non-React JavaScript touched React nodes and broke everything.

[jquense]

This seems like it would create annoying noise in some instances. There are a few use cases where direct DOM mutation is desirable, or just represents the actual simpliest implementation. Animation is definitely one such case. Also, as mentioned, any wrapping of none react library widgets (jquery ui,etc).

I wouldn't want false positives to drown out real warnings, maybe it can be turned on, or hinted to not apply for certain components?

[Daniel15]

This seems like it would create annoying noise in some instances. There are a few use cases where direct DOM mutation is desirable

IMO those cases should be explicitly marked as such (perhaps a dangerouslyAllowExternalMutations attribute or something) as it's quite rare.

[jquense]

I am fine with opting out, as it encourages the general better behavior, as long as i don't need to have React chastising me when i know what i'm doing :P in the cases where it makes sense. The downside is that you are adding more API surface area with a dangerouslyAllowExternalMutations, albeit a small bit. Still seems sort of silly to add api for development warnings.

[syranide]

Mutation of DOM node attributes/properties is safe (but should be avoided). Mutation of the DOM node hierarchy for anything but insertion of nodes into empty leaves is disaster waiting to happen. There shouldn't be an issue discerning an unsafe mutation from a safe.

[Axbon]

This would be very useful, in particular when introducing React in legacy codebases when perhaps something like jQuery/dojo/x still exists but the idea is to get rid of them over time.

[j0lv3r4]

Since v15.0 data-reactid is not used anymore, but I assume this feature should work with older versions as well, right?

[syranide]

@thinkxl This is unrelated to data-reactid and still something that makes sense to do.

[j0lv3r4]

@syranide right, I was making a reference to @jimfb 's suggestion:

As discussed in #3207 (comment), a complementary improvement is to turn on a mutation observer in dev, and warn of someone other than React modifies the DOM and/or if dom nodes appear without a data-reactid

And trying to figure it out without data-reactid, but I think I didn't elaborate the question correctly, sorry about that.

I was thinking of use MutationObserver() API but ReactDOM.render() would trigger the callback.

My goal was to make a PR for this but I'm out of ideas without data-reactid 😅

[syranide]

@thinkxl It still exists internally via the same mechanisms as before, you just can't explicitly check for the attribute itself.

[jimfb]

I was thinking of use MutationObserver() API but ReactDOM.render() would trigger the callback.

@thinkxl The mutation observer would trigger a devtool, and the devtool would track if a sanctioned DOM operation was currently in progress. Just ignore DOM mutations that occur when React is performing a DOM operation.

[SalehHindi]

@syranide, @jimfb, @thinkxl, are either of you working on this issue? I'd like to take a crack at it if possible. If you already wrote some code or made a pull request, could you link it here?

[j0lv3r4]

@SalehHindi I haven't had time to work on it yet, go a head, I'm curious on how you would approach this 😄

[syranide]

@SalehHindi I'm not aware of anyone working on this :+1:. If you're not sure what the best way forward with an implementation is then feel free to "write down your idea" and we can comment on that as well.

[SalehHindi]

@syranide, thanks for the quick reply. I'll be sure to work on this tonight and write my idea here. If anyone else wants to help, let me know/

[SalehHindi]

@syranide @Daniel15 @thinkxl, whoops, it looks like I accidentally pushed to facebook/react instead of my local fork! I understand this is a very rookie mistake and I am very very sorry. I tried fixing the commits, but it looks like I pushed the same exact commit instead of reverting to an earlier commit. I even tried deleting my fork to double check if it's on facebook/react. Since this is the master branch, I wanted to know if anyone can help undo these commits because I don't want to error and revert back too far and make a permanent change.

Again, super sorry and this will never happen again.

[syranide]

@SalehHindi We've all made our fair share of git oopsies. But you have nothing to worry about, the changes you have made were most likely to the react-master branch of your fork, you do not have the permissions to push to the official repository.

If you want to fix it and keep your changes, the best way is to simply go to the master branch, create a new branch, then reset the master branch back to before your commit. You can do all your changes in the master branch, it's ok, but it makes it hard for you if you wish to submit further PRs.

[SalehHindi]

https://gist.github.com/SalehHindi/f6599739c3d51719d8944bffe10ae6c3#file-reactclass-js-L763

This is my attempt but I am having trouble with a couple of things. I created a window.startObservation() function in ReactClass.js because I figured any page that has a react element needs to call createClass(). I'd be happy to answer any questions about my implementation.

My biggest problem is that although this code observes for removal of react nodes and addition of nodes under each react node, my code isn't able to determine whether react is the source of the mutations. This means that if react adds or removes a node as intended, this would trigger a mutation warning. Additionally, this means that I couldn't figure out where to place window.startObservation() because if I call it before all the render calls, as soon as react renders its elements, it would trigger a mutation. So for now, I had a test page with some react elements and I call window.startObservation() after all the render calls.

Another issue is that if I am observing an element and delete a parent high up in the chain -- like the body node -- then it will not a trigger a mutation change. A viable workaround is to observe all nodes on the page and then on mutation, check if the mutated element or any of its children are react elements.

Any ideas on how to fix the first issue? I'd be very happy if someone could help or critique my work.

[syranide]

@SalehHindi I haven't read your code, but here's some feedback.

because I figured any page that has a react element needs to call createClass().

Incorrect, there are many different ways to create classes, there are even pure functional components (just functions).

My biggest problem is that although this code observes for removal of react nodes and addition of nodes under each react node, my code isn't able to determine whether react is the source of the mutations.

The solution to this will be the same as to the above issue. This needs to move into the "core" of ReactDOM. Essentially, you should wrap and start/stop the observer around DOMChildrenOperations (or something a bit up the call chain most likely, but you get the idea). This is your ticket to knowing when React is performing mutation vs the user.

then it will not a trigger a mutation change. A viable workaround is to observe all nodes on the page and then on mutation, check if the mutated element or any of its children are react elements.

Unless I'm mistaken there are two different scenarios here, invalid mutation of node hierarchies owned by React (big no-no, will crash) and React nodes being manually discarded rather than correctly unmounted (bad, but less important). The focus of this PR should be the first IMHO.

Considering there may be multiple React trees rendered inside each other, it probably makes sense to have a single global observer rather than one per tree as the overlap may become problematic, maybe.

@spicyj @jimfb What's a good test for seeing if mutation is allowed. If the affected node is moved/etc and owned by React OR (target parent is owned by React AND any of its target siblings is owned by the same React tree) ... then that's an error? (EDIT: my rules are kind of ambigious, but I think you get the idea)

[SalehHindi]

I think I fixed this. At what point should I open a pull request? @syranide, should I discuss changes I made here first?

[jimfb]

If you already have the code written, there is no harm in opening a PR. If you'd like to discuss and/or ask questions before writing the code, that may save you some time/effort/energy.

[j0lv3r4]

the PR got closed but the fix wasn't merged, is this still needed?

[aweary]

@thinkxl the PR was closed since it got outdated, I believe we're still open to accepting a PR that implements this if anyone wants to work on it!

[j0lv3r4]

@aweary cool, thanks for the quick response

[gaearon]

We might still want to do it, but an implementation proposal should clearly explain which mutations are dangerous, and how to detect them and avoid false positives. I closed https://github.com/facebook/react/pull/8829 since it didn't attempt that.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale[bot]]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!