Copyless setup of react source

[byroot]

Copying files on boot is really problematic, especially in capistrano scenario where the application run under a different user than the one compiling assets.

e.g. the deploy user compiles the assets and the rails user can only read them.

The current copying behaviour requires a lot of capistrano hacks, and is also unsafe because the running application if compromised could corrupt the asset bundles.

My proposal is not to copy files but to append different directories to sprockets search path. To work, this would require to change the paths in the react-source gem to:

• build/JSXTransformer.js
• development/react.js
• development-with-addons/react.js
• production/react.js
• production-with-addons/react.js

@rmosolgo thoughts about this?

cc @gmalette

[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 [email protected]. Thanks!

[rmosolgo]

😹 The same idea hit me while writing up #252 , I love it.

It looks like this removes support for the "enviroment drop-in path" and that's fine with me -- seems like a rare use case with a trivial workaround of just dropping a copy of react.js in your /assets/javascripts/vendor directory (whatever pleases you)

You'll probably have to update a couple tests, too. Thanks, this is awesome!

[byroot]

I'm glad you like it. I'll update the test and PR the related change to react-source

[rmosolgo]

Oh I missed the changes to react-source. If you don't feel like altering react-source, I'm open to a rake update_react task that updates the gem, then copies the those files into the react-rails source.

So then to add different versions, you'd just add different react-rails asset paths to the sprockets environment (instead of react-source paths).

Your call, I love the copy-free setup either way

[byroot]

I already started a PR on react-source, and it feels like the cleanest solution anyway. I'm no grunt expert though so it's not easy :p

[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!

[byroot]

So @rmosolgo, On second though the rake task is probably simpler and do not require to release react-source nor to break it's compatibility.

So I pushed 8e16728 that does just that. The same tests are still broken, but I'll see what I can do to fix them.

[zpao]

👍 I was going to ask in the other PR about compatibility.

The other option we have here is to stop using react-source entirely. I don't know how useful it actually is at this point. When I first started this project I modeled it after a couple other things (combo of coffeescript and ember) but didn't know the implications.

What we do with pyreact is actually just download the js files from https://github.com/reactjs/react-bower into that repo and then there aren't the constraints of another dependency. If there's no value in react-source, then I'm happy to just kill it, let me know.

[byroot]

Well, actually I moved react-source as a development dependency only since It's not needed at runtime anymore.

So yes using the same strategy than pyreact sounds good. I'll do that then, it will simplify even more.

[byroot]

Ok, so I vendored everything using bower. I also fixed the "environment drop-in path" test case (the rails app assets directories have priority over the gem).

There is 2 remaining broken test cases, but I don't really understand them. @rmosolgo your help would be appreciated to figure out what wrong with thoses.

[byroot]

Ok, so out of the 2 failures, one already fail on master for me, so it must be a problem with my setup.

As for the second one (drop-in files), I think I already cover that in another test. The coverage lost though is the test that assert that the development version is served. Not sure if it's critical.

[rmosolgo]

How does this work with a new project?

If it's a new project, lib/assets/react-source/ would be empty, right? So adding those directories to the asset pipeline wouldn't be much help. I'd rather not make a new user run a rake task just for setup (that would bring back the file copy problems).

I see that lib/assets/react-source is gitignored, but what if we actually committed that to the repo and distributed it in rubygems? In fact, I think we could commit that instead of vendor/react, since the gem doesn't depend on those files.

That way, the rake task would be part of react-rails maintenance. To update the vendored JS, you'd run the task, which:

• gets the latest from bower into git-ignored directories
• copies versions into lib/assets/react-source (or wherever)

This introduces a hidden dependency on bower, but hey I guess that's the downside of this approach. Worth it IMO.

What do you think of that? Have I missed something?

[byroot]

If it's a new project, lib/assets/react-source/ would be empty, right?

Not sure what you mean by that. lib/assets/react-source/ stays in the react-rails gem, not in the hosting app. But whatever since your next point is good.

I think we could commit that instead of vendor/react, since the gem doesn't depend on those files.

That's a good point and it also fix one issue I have with the current state of the PR, which is that you can't use it as a git gem because the files are created during the build.

I'll update this PR in a minute. Hang on.

[byroot]

@rmosolgo should be good now.

The rake react:upload task will update react to the latest released version and copy files in the proper place.

[byroot]

For the record as just tested this branch in my app as a git gem, and it dies work as intended.

[rmosolgo]

That looks nice! Bower is just the means for getting those files into the right directories. Then, the Railtie switches on config.react.variant and adds the desired directory to the asset pipeline.

The only remaining thing is this: is there any test that the directory approach really works? Eg testing production vs. development, testing add-ons vs no add-ons? (I'm not 100% sure how to test it)

This removes support for environment-based drop-in react.js files but that's OK with me. I've never heard anyone using that configuration. I'll create a changelog after this PR is merged.

[byroot]

The only remaining thing is this: is there any test that the directory approach really works?

It's really hard to test since sprockets caches everything. But I can give it a try.

On a side note. It seems that the path is appended too late. I tried deploying 07eec91 and assets:precompile is failing in production:

TypeError: can't modify immutable index
.../shared/bundle/ruby/2.1.0/gems/sprockets-2.12.3/lib/sprockets/index.rb:81:in `expire_index!'
.../shared/bundle/ruby/2.1.0/gems/sprockets-2.12.3/lib/sprockets/base.rb:109:in `append_path'
.../shared/bundle/ruby/2.1.0/bundler/gems/react-rails-07eec911ed95/lib/react/rails/railtie.rb:39:in `block in <class:Railtie>'

I'll find a way to fix this.

[rmosolgo]

Not sure if this is related, but it rings a bell: #241

[byroot]

260cb28 fixed it. It's just that sprockets freeze it's environment on after_initialize.

I successfully deployed 260cb28 in production.

[byroot]

Grumble. Of course that change broke the same tests again....

I'm quite busy today, but I'll fix the tests as soon as I have a bit of time.

[rmosolgo]

@byroot I tried another way of adding those assets, and I also added a test for the precompile task (only makes sure no 💥), here's a PR into your branch: byroot#1

Almost there!

[byroot]

f5e47ed fixes the new precompile test under 3.2.

[byroot]

And 1c92a45 test that it's the development version that is loaded. I don't know How I could assert that different settings load different version since it would require to reboot rails :/

Would that be good enough for you?

[rmosolgo]

👍 from me! I'll give it a try locally before I merge it and let it hang here for a bit in case anyone else has suggestions.

[rmosolgo]

I gave it a try locally:

• pointed at

    gem 'react-rails', #  '~> 1.0'
      github: "byroot/react-rails",
      branch: "dont-crash-on-boot-because-of-file-copying"

• tried getting :development and :production variants locally

• at first, I had trouble -- even after changing to :production, I was still getting the development version

• I cleared my sprockets cache (manually deleted tmp/cache in my case), then it worked:

  

👍

[byroot]

Oh the cache issue is interesting, I totally didn't think about it.

It's probably a minor issue, but quite annoying. What we could do is to append the react-rails settings to the sprockets environment version: https://github.com/rails/sprockets-rails/blob/656c9d266a0c5d29d19c566831c8d7daa2f2005d/lib/sprockets/railtie.rb#L91-L97

[byroot]

I think byroot@b3ee527 would solve the issue. Let me know if you want it included in this PR.

[rmosolgo]

would be nice as a separate PR!

On Mon, May 4, 2015 at 2:16 PM, Jean Boussier [email protected]
wrote:

I think byroot/react-rails@b3ee527
byroot@b3ee527
would solve the issue. Let me know if you want it included in this PR.

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

[rmosolgo]

what a huge win, thanks @byroot 🎊 🎉 !!

[byroot]

My pleasure :)