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

Implement server-side rendering #24

Merged
merged 1 commit into from Apr 13, 2014

Conversation

Projects
None yet
10 participants
@johnthethird
Copy link
Contributor

johnthethird commented Feb 10, 2014

Add an additional view helper react_server_component with the same interface/functionality as react_component with the exception that the body of the tag will be filled with a server rendering of the HTML markup. (Im not sure if it should be a different method name or perhaps just an argument to react_component? )

For thread-safety we use the ConnectionPool class from @mperham to control access to the JS VMs. It is performing well under load in my tests.

end

def render(component, args={})
# This works because even though renderComponentToString uses a callback API it is really synchronous

This comment has been minimized.

@zpao

zpao Feb 10, 2014

Member

This isn't true anymore, it's sync in master which will be out in 0.9.

@quark-zju

This comment has been minimized.

Copy link
Contributor

quark-zju commented Feb 10, 2014

Follow existing pattern of Rails, I think something like :prerender => true in options is better than another method.

And here is a even more fun/crazy idea taking advantage of the existing method render:

react_component(:Foo) # render in client
render react_component(:Foo) # pre-render in server

This is tricky. But it is implementable by returning a fake Hash object with a special to_s and providing special partial_path, locals. However, it is probably confusing users so don't acturally do it.

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Feb 17, 2014

OK, I rebased everything on top of master, and force-pushed (not sure if that was a faux pas). I implemented @quark-zju suggestion for :prerender => true instead of a separate view helper . I also made it so it should work with 0.8 and 0.9 versions of the renderComponentToString API.

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Feb 17, 2014

Yeah, you have to force-push after rebase.
I think it would be valuable to be able to pass data (say JSON) to the component that will be rendered, not sure if you thought about that?

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Feb 17, 2014

@jakubmal Yes, the view helper react_component takes a component name, and an args hash that will be passed in to the component as the initial props. This works both for server-side rendering and the client-side rendering. So something like

<%= react_component "TodoList", {:todos => ['todo1', 'todo2', 'todo3']} %>

gets turned into

<div data-react-class="TodoList" data-react-props="{&quot;todos&quot;:[&quot;todo1&quot;,&quot;todo2&quot;,&quot;todo3&quot;]}">
  ...
</div>

and then the react_ujs.js can unobtrusively turn that into a React component.

@robertfall

This comment has been minimized.

Copy link

robertfall commented Mar 20, 2014

I just want to chime in and say I would love to see this added to react-rails.

We're very keen to start using this fork, but obviously sticking to the authoritative source is more supported in the long run. Is there something holding up the Pull Request being merged in that I can help with?

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Mar 20, 2014

@jakubmal, what do you think?

@zpao zpao added the CLA needed label Mar 20, 2014

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Mar 20, 2014

Also, @johnthethird, could you sign the CLA?

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Mar 20, 2014

@zpao no problem, CLA has been signed.

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 21, 2014

@zpao will let you know here over the weekend

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 22, 2014

@robertfall this PR is tied to 1.0.0.pre, and it (1.0.0.pre) needs to be ironed out a bit first.

@robertfall

This comment has been minimized.

Copy link

robertfall commented Mar 22, 2014

So once 1.0.0.pre is finished up there should be no problem merging this in? Unless of course changes in 1.0.0.pre necessitate changes in this PR.

Is there anything we can help with for 1.0.0.pre?

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 22, 2014

@robertfall I'm testing this PR this weekend, if it's okay it'll be merged right after rebasing. See issues for problems that need to be reviewed before going 1.0.0.rc / 1.0.0

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 22, 2014

@johnthethird could you rebase as soon as you can? I don't want to leave this hanging for too long and I'm pretty sure other PRs will be conflicting. I'll confirm this is good to go sometime today or tomorrow, but in the meantime it would be great if you rebased.

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Mar 23, 2014

@jakubmal OK, I rebased, added a commit to change to the sync version of renderComponentToString, ran the tests, and pushed.

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 24, 2014

@johnthethird
First, thanks for this! It is going to add a great value to our little react-rails! And surely you put a lot of effort into this.

I think we'll be able to iron out a couple of issues that I highlighted soon.
Also, don't hesitate to correct me wherever I'm wrong 👍

@bensmithett

This comment has been minimized.

Copy link

bensmithett commented Mar 24, 2014

I'm having issues running a pretty simple sample app in production mode, getting this output. Don't know enough to know if I've done something stupid or if this is a real issue, either way thought I'd flag it here in case the README needs tweaking to avoid this.

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Mar 24, 2014

@jakubmal I will review your comments and make any appropriate changes. Also, I was able to reproduce @bensmithett issue, so I will take a look at that as well.

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Mar 25, 2014

I changed the way the initializer works, which should also fix issue #30 and #31as well. @bensmithett can you give it a try and see if it works for you now?

@bensmithett

This comment has been minimized.

Copy link

bensmithett commented Mar 25, 2014

@johnthethird yep all good now 👍

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 28, 2014

@johnthethird could you PR 940c7f2 separately, if you don't mind? I doesn't really fit this PR...

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Mar 28, 2014

@johnthethird I just mailed you, why did you close this?
EDIT: I'm not sure what happened here - please email me if something is not right!

@johnthethird johnthethird reopened this Mar 31, 2014

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Mar 31, 2014

I force-pushed a new commit for this feature, re-based on the work I teased out to fix #31. It would certainly look cleaner to just open a new PR, but I figured the conversation here was probably worth something.


# Watch .jsx files for changes in dev, so we can reload the JS VMs with the new JS code.
initializer "react_rails.add_watchable_files" do |app|
app.config.watchable_files.concat Dir["#{app.root}/app/assets/javascripts/**/*.jsx*"]

This comment has been minimized.

@keithpitt

keithpitt Mar 31, 2014

This should also watch for files that match: "*.js.jsx.coffee"

@zpao zpao added CLA signed and removed CLA needed labels Mar 31, 2014

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Apr 9, 2014

This is getting lengthy, just as I didn't want. I think it'll be sensible to just merge as soon as you rebase, and then work iteratively from there. @johnthethird what do you think?

@johnthethird

This comment has been minimized.

Copy link
Contributor

johnthethird commented Apr 9, 2014

@jakubmal OK, rebased and ready to merge into master.

@petehunt

This comment has been minimized.

Copy link

petehunt commented Apr 11, 2014

pingping

@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Apr 11, 2014

Hi! @petehunt This should get merged today 👍

jtmalinowski added a commit that referenced this pull request Apr 13, 2014

Merge pull request #24 from johnthethird/server-rendering
Implement server-side rendering

@jtmalinowski jtmalinowski merged commit c09a0c6 into reactjs:master Apr 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jtmalinowski

This comment has been minimized.

Copy link
Contributor

jtmalinowski commented Apr 13, 2014

I'll submit unresolved problems from here as new issues to streamline working on this.

@keithpitt

This comment has been minimized.

Copy link

keithpitt commented Apr 13, 2014

This is so very awesome. Thank you guys for your awesome open source work!

Internet high fives!

@bensmithett

This comment has been minimized.

Copy link

bensmithett commented Apr 14, 2014

😄 🎊 what @keithpitt said, thanks guys!

@nelix

This comment has been minimized.

Copy link

nelix commented Apr 15, 2014

<3 been waiting for this one

@knwang

This comment has been minimized.

Copy link

knwang commented Apr 30, 2014

very excited about this!

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