Skip to content

Conversation

xionon
Copy link
Contributor

@xionon xionon commented Jul 14, 2014

[Heavily based on the ember.js bootstrap generator]

Running rails generate react:install will append the basic require directives to application.js

Possible avenues for upgrades:

  • allow for files with arbitrary names ("mycomponents.js" instead of "application.js")
  • allow for files with different extensions ("application.js.coffee" instead of "application.js")

@zpao
Copy link
Member

zpao commented Jul 31, 2014

I think this is a great idea and would be a welcome addition.

I started with keeping the folders in lib modeling the modules they're named after (I thought that was a Ruby gem pattern). I don't really care either way, just want to make sure we follow whatever best practices are. But I haven't written much Ruby in the past few years outside of the initial version of this gem, so I'll defer to others.

@xionon
Copy link
Contributor Author

xionon commented Jul 31, 2014

Sorry, I guess my comment wasn't very clear. This pull request actually adds the generator for users that have installed the gem into their rails app, which is very similar to how ember-rails works.
The idea is to make installation and usage easier for Rails developers, especially those new to React. I have ideas for ways to enhance the generator, and I was looking for feedback about whether or not adding Rails generators was a direction you wanted this project to go in.

After merging this pull request, users who install the gem in their Rails app will be able to run rake generate react:install. This will create the components directory, create a components.js file, and directives to require react, react_ujs, and components.js to the application.js file.

The generator currently does not support CoffeeScript or custom names for application.js.

One of the additional generators I'd like to build is rails generate react:component MyComponent, which would scaffold out a file in app/assets/javascripts/components/my_component.js.jsx -- but if you're not interested in adding generators to this project (which is totally reasonable), I'll build it as a separate gem. 😄

@zpao
Copy link
Member

zpao commented Aug 1, 2014

No, sorry I wasn't clear! I think generators would be awesome.

I was just getting nitpicky on the file path. I was thinking lib/generators/react should be lib/react/generators (to mirror React::Generators). But this is where my lack of Rails/Ruby expertise shows - I don't know what best practices are for files + classes/modules, or if there's actually a requirement for Rails.

@xionon
Copy link
Contributor Author

xionon commented Aug 1, 2014

Oh, gotcha.

lib/generators/ is where Rails looks to automatically add generators to the rails generate list. AFAIK, it's not just a convention, but a requirement if you want Rails to automatically load them. Even if it were just a convention, though, I would recommend sticking with lib/generators/react, because that's where bigger gems like devise, ember-rails, and backbone-on-rails stick their generators.

[Heavily based on the ember.js bootstrap generator]

Running rails generate react:install will append the basic require directives
to application.js

Possible avenues for upgrades:

  - allow for files with arbitrary names ("mycomponents.js" instead of
    "application.js")

  - allow for files with different extensions ("application.js.coffee"
    instead of "application.js")
@jtmalinowski
Copy link
Collaborator

I'm just not sure whether it's super good to keep components.js thing as is - we are effectively suggesting that React components in your application should not be dependency-injected, but I guess FB @zpao doesn't have any recommendations for resolving dependencies in React apps right?

@xionon
Copy link
Contributor Author

xionon commented Aug 5, 2014

If there's a better alternative, I'm happy to pull out the components.js
bit and leave that up to the developer. My goal is to make things easy for
new react users, so if it can be scaffolded on install, I'd like to
implement it. But I'm having trouble visualizing how js dependency
injection would work with the rails asset pipeline and server prerendering.

Can you give me an example of what you're thinking, even in pseudo-code? Or
if there's an example JS app using DI and React that I could look at, that
would work too.

On Tuesday, August 5, 2014, Jakub Malinowski notifications@github.com
wrote:

I'm just not sure whether it's super good to keep components.js thing as
is - we are effectively suggesting that React components in your
application should not be dependency-injected, but I guess FB @zpao
https://github.com/zpao doesn't have any recommendations for resolving
dependencies in React apps right?


Reply to this email directly or view it on GitHub
#64 (comment).

@jtmalinowski
Copy link
Collaborator

commonjs would be good #33 but maybe we should just proceed with this and then iteratively improve...

react_ujs and the autorequiring of components is new in master. The
install generator now accounts for some application.js files already
having react installed.
@xionon
Copy link
Contributor Author

xionon commented Aug 6, 2014

@JakubMal I agree, supporting additional gems like maccman/sprockets-commonjs is outside the scope of this ticket.

@zpao
Copy link
Member

zpao commented Aug 6, 2014

JS modules (usually commonjs) are what we always recommend. I don't know what best practices are in Rails land for building JS bundles based on dependencies & that sort of thing. Browserify & co have picked up a lot of steam in the node community.

There are always going to be issues with the view helpers so I think just be opinionated and build what seems to work best.

@xionon
Copy link
Contributor Author

xionon commented Aug 6, 2014

@zpao TL;DR - CommonJS would be really nice, but it's not the "Rails way." I would love for that to change, but that's a long ways (and a separate gem) away. For now I would recommend sticking with components.js.

Rails does not have a good story for integrating Browseify or CommonJS modules. Instead, Rails uses a version of Sprockets -- the integration is referred to as the asset pipeline. As I remember it, Sprockets was built before node.js really took off, and it's stuck around mainly because that's what Basecamp uses.

Dependencies can be managed using //= require from any file. If two files require the same dependency, it will only be included once. However, by default it's all loaded into the global context - there's no built-in equivalent to module.exports, and I think trying to implement that by default would confuse more people than it would help.

It's possible that you could do things like this:

<%= react_component 'Bar' %>
//= require react
//= require components/foo
var Bar = react.createClass({
  render: function() {
    return <div><Foo /></div>;
  }
});

If the view helper then only asked the asset pipeline for the components/bar.js file, the asset pipeline could figure out the dependencies on react and foo and bundle those together. However, I think this is a really bad, Un-Rails-y way of doing it. It would break a lot of user's assumptions about how the asset pipeline works, and probably would end up building a lot of different js files and holding them in memory.

The asset pipeline has a build process, where the //= require directives from a mainfest file (by default, application.js) are parsed and concatenated into a single static file on deploy. You can add more manifest files to the list of files to build, and I think it would make sense for this gem to automatically add components.js to that list. That way, application.js and components.js would both be built on deploy, and components.js could be used separately from the rest of the js.

@zpao
Copy link
Member

zpao commented Aug 6, 2014

👍 Thanks for the explanation. I knew some of the asset pipeline stuff, just not sure if it had really changed in the past few years. Sounds like no. The biggest JS-related innovation seems to be Turbolinks and that's really just to make is easier to continue using server-side views.

I don't know how any Rails apps with non-trivial JS survive, unless they all use coffeescript and get IIFEs for free. Still needs to be globals somewhere if these bits of code are supposed to talk to each other. There's a lot of room for error with straight file concats but that's what we have.

If we want to be good Rails citizens (and I think we do), let's do it their way and use... globals and I guess components.js. At least you can reference objects so react_component 'Foo.Bar' works and you can group things slightly more sanely without completely polluting the global scope.

@xionon
Copy link
Contributor Author

xionon commented Aug 6, 2014

All of your observations and assumptions about Rails projects using non-trivial amounts of Javascript are correct. Turbolinks is flaky, and CoffeeScript allows them to punt on a real JS strategy for a few more years. That said, the situation is much improved from the bad old days, where you just stuck your JS in the public directory grab-bag and Rails dropped in 10-15 individual <script> tags per page.

I'd love to use CommonJS, and if I were building a large new app from scratch, I'd probably disable the asset pipeline altogether and use something like Broccoli for front-end stuff instead. But I also have to work on legacy projects where the asset pipeline is already working, and I think react-rails could make the most sense for those apps.

@jtmalinowski
Copy link
Collaborator

Sprockets works fine for Angular apps - it can work fine for us too. But let's proceed with global namespacing for now.

@xionon
Copy link
Contributor Author

xionon commented Aug 9, 2014

OK, so, are there any changes I need to make before this can be merged?

@zpao
Copy link
Member

zpao commented Aug 12, 2014

Probably want to add usage to the readme. Apart from that I think this looks fine. Any other comments @JakubMal?

@jtmalinowski
Copy link
Collaborator

+1 for readme, no more comments

@xionon
Copy link
Contributor Author

xionon commented Sep 4, 2014

Apologies for disappearing and letting this drop.

I added the smallest amount to the README that would be necessary to communicate this. I'm not sure an install generator really needs much more than that.

zpao added a commit that referenced this pull request Sep 7, 2014
[feedback requested] Create a installation generator
@zpao zpao merged commit 62f9a71 into reactjs:master Sep 7, 2014
@zpao
Copy link
Member

zpao commented Sep 7, 2014

Thanks, I think this is a great addition! I'm sure we'll here if anything goes wrong so keep an eye out.

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

Successfully merging this pull request may close these issues.

3 participants