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

Add node as an option to the generator #469

Merged
merged 4 commits into from
Aug 21, 2016

Conversation

jbhatab
Copy link
Member

@jbhatab jbhatab commented Jul 9, 2016

Node can be used as an alternate to rails for rendering. It requires a few files to set it up so we added it to the installer to make this even easier to setup.

This change is Reviewable

@jbhatab jbhatab force-pushed the feature/add-node-option-to-generator branch from 9a4400c to 32f5d13 Compare July 9, 2016 02:16
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 82.638% when pulling 32f5d13 on feature/add-node-option-to-generator into 87d2fc3 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 82.638% when pulling 32f5d13 on feature/add-node-option-to-generator into 87d2fc3 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 82.638% when pulling 32f5d13 on feature/add-node-option-to-generator into 87d2fc3 on master.

@justin808
Copy link
Member

@robwise @alleycat-at-git What's the best way to verify this? I'm guessing that @jbhatab tried this and he got the node generator to work.

@alexeuler
Copy link
Contributor

alexeuler commented Jul 9, 2016

@justin808 @jbhatab I tried to check it, but failed to set it up. Here are the key points:

  • Basically when I run the generator I still had a server rendering set to ExecJS in react_on_rails config (this must be fixed to NodeJS in this generator).
  • Then it's important not to forget to turn on server rendering options in component rendering (<%= react_component("HelloWorldApp", props: @hello_world_props, prerender: true) %>). I'm not sure if we have to set it in the generator, but probably yes.
  • I think it makes sense to add a node process to Procfile.dev to start it up automatically when we run npm run rails-server
  • It would be nice if we could restart the Node process when the bundle changes (right now we have to restart it each time manually). Or even better if we could preload the initial node process into memory and fork it each time the bundle changes (or we have a request to a stale bundle)

@justin808
Copy link
Member

@jbhatab

  1. It would be nice if we could restart the Node process when the bundle changes (right now we have to restart it each time manually). Or even better if we could preload the initial node process into memory and fork it each time the bundle changes (or we have a request to a stale bundle)

That's pretty majorly bad for development and will surely lead to some confusion. Let's put a note in this file: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/node-server-rendering.md

and let's cross reference it here: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/server-rendering-tips.md

@justin808
Copy link
Member

@jbhatab Let's get this one wrapped up!

@justin808
Copy link
Member

@jbhatab Any update?

@justin808
Copy link
Member

@jbhatab @alleycat-at-git Is this ready for merge? @jbhatab Please rebase on top of master.

@jbhatab
Copy link
Member Author

jbhatab commented Aug 1, 2016

@justin808 No I need to address the comments @alleycat-at-git made.

@justin808
Copy link
Member

@jbhatab What's going on with this PR?

@alleycat-at-git Did we update the docs and the generator file for your fix to the size of the request?

@justin808
Copy link
Member

@jbhatab Should this get merged? We're about to do a release!

@justin808 justin808 modified the milestone: 6.1 Aug 8, 2016
@justin808
Copy link
Member

I'd like to get this in the 6.1 release this coming week.

@justin808
Copy link
Member

@jbhatab @alleycat-at-git @robwise Is this one good to merge, once rebased on top of master?

@robwise
Copy link
Contributor

robwise commented Aug 21, 2016

@alleycat-at-git Did we ever fix the bug with using sockets?

@jbhatab
Copy link
Member Author

jbhatab commented Aug 21, 2016

I addressed what @alleycat-at-git mentioned. Thanks for those, good catches.

Should be good to merge, but feel free to look it over again @justin808.

@justin808
Copy link
Member

@jbhatab We still need @alleycat-at-git to confirm that you made the changes that address the large data sets for server side rendering.

@justin808
Copy link
Member

Minor comments.


Reviewed 5 of 9 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


CHANGELOG.md, line 7 [r2] (raw file):

## [Unreleased]
- Node option for installer added as alternative for server rendering.

Take credit @jbhatab,. See other entries.


README.md, line 16 [r2] (raw file):

# NEWS
* 2016-8-30: 6.0.6 added node option to the installer to easily setup alternative server renderer.

Let's remove this line. I don't want to emphasize this.


lib/generators/react_on_rails/templates/node/base/client/node/package.json, line 10 [r2] (raw file):

  "dependencies": {
  }
}

new line!


Comments from Reviewable

@justin808
Copy link
Member

:lgtm:


Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@justin808 justin808 merged commit 63aa9bc into master Aug 21, 2016
@justin808 justin808 deleted the feature/add-node-option-to-generator branch August 21, 2016 06:46
@AlexKVal AlexKVal mentioned this pull request Aug 21, 2016
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.

5 participants