Expose the React mounting and un-mounting mechanisms

[robrobbins]

Re: #116

This is my initial commit, all within a react_ujs_p.js file so that we may discuss it further. I'll get to the tests tomorrow, though I expect the existing to pass. I do need to write a few new ones.

Currently I am using this modded version is a non-turbolinks SPA and it works fine.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[rmosolgo]

👍 I know there are some other PRs out there regarding turbolinks compatibility and events but this seems like a solid improvement on master. React.ujs seems good to me.

I see you added a bit about removing data-react-class node when unmounting -- seems fine with me, in my cases that node is always invisible and non-structural or whatnot.

[robrobbins]

Those data-react-class divs can really add up in a SPA, I felt that removing them when calling ...unmount was the most expected behavior. I don't use Turbolinks so can't speak to that compatibility - maybe other collabs will chime in there, but as you said this code would not impact that.

My question would be: Do we want to keep 2 different ujs files or just replace react_ujs.js with this one?

[rmosolgo]

seems right to replace the previous one with this one, right? It's the same code (with that bit added), just assigned to React.ujs

[robrobbins]

Sounds good to me. I'll delete the ..._p.js and replace react_us.js with it, write a couple specs to assert the presence of the methods and props and re-PR.

need to do some paying work first, so e.t.a on that tonight sometime

[robrobbins]

Replaced the primary ujs file. How do you want to proceed to setup a test to inspect that exported object. My thought was to simply get a global of some sort setup where we could assert a few things...

[robrobbins]

Added tests to the view_helper that looks for the presence of all the exported methods and attributes. All green.

[rmosolgo]

👍

[zpao]

Honestly, I'd rather we didn't add things to React.* at this point as the functionality provided here is distinct from the rest of React and could lead to confusion/problems down the line (it's unlikely in this case but there are other properties we may want to add to React without potential issues).

[robrobbins]

That's fair, but raises another question: Where then? I have no strong feelings about where, just that they need to be exposed somewhere. Ideas?

Reason I put them there, risking stating the obvious, is that you are using React + Rails. So the appended namespace is not a surprise?

[rmosolgo]

how about a global like ReactRailsUJS ?

[robrobbins]

I'll put in whatever consensus wants. Is there a precedent with any other UJS things?

[rmosolgo]

@zpao I think we're just waiting on your OK or other suggestion on these 2 questions. It would be nice to get this worked out, then update those other PRs

[robrobbins]

Yep, I can proceed as follows (just want the consensus first, so as not to do it again):

• expose the api as ReactRailsUJS (or other...) not on React
• leave the $ (return jQuery || Nodelist)
• fix the style and whitespace issues
• fix the specs obviously

I think thats it?

[rmosolgo]

@zpao any objections to going forward like that ^^ ?

@robrobbins He said he'd be busy so if we don't hear from him in 48 hours, I think we should take that as agreement :)

[robrobbins]

sure. i'll get on it later today then

[zpao]

👍 sounds good! Sorry for the delay, this project is unfortunately low on the priority list with everything else :(

[robrobbins]

Done

[rmosolgo]

👍 you're a good man @robrobbins

dropped this in locally just to make double-sure & looks good to me

[robrobbins]

👍 Thanks

Did the same, been using it on the redesign of a big commercial app. Doing fine.

[rmosolgo]

💸

[jmagoon]

This is a great addition. Quick question, is there any reason to leave the JSON inside of the data-react-props attribute after you've called React.render()? My current application renders about 5000 objects to the page, and having those sit in the attribute causes the developer console to crawl and sometimes crash.

My current approach is just to call node.removeAttribute(PROPS_ATTR) right after the render step in the MountComponents function. Is there a reason to keep that attribute around?

[rmosolgo]

@jmagoon I'd be in favor of that change.

The only downside I can think of is that components wouldn't be re-mountable. But, is that something that matters? Does anyone depend on that?

Actually, I think I do that sometimes: arbitrarily fire page:change (faking the Turbolinks event) in order to mount components in a just-loaded modal. At present, it just doesn't matter because it re-mounts with the same data-react-class and data-react-props (and React doesn't end up doing anything to the DOM). So maybe if you removed data-react-props, you could also remove data-react-class, so that calling ReactRailsUJS.mountComponents() would just miss those things all together.

Oh wait, that wouldn't work because unmountComponents depends on that data-react-class attribute. :S

Anyways, I'm open to something like that ^^, especially if we could get something like #91 worked in. (Then I could stop my stupid hack for mounting new components!)

[robrobbins]

i can look into it. @jmagoon @rmosolgo you guys wanna open an issue with those things summarized? would prob help me to remember, seems like something that could be done fairly easily (famous last words...)

[rmosolgo]

@jmagoon Turns out we need to keep those props in the DOM to support turbolinks, see #159