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

Only require rails as a development dependency #427

Closed
wants to merge 2 commits into from

Conversation

natemontgomery
Copy link
Contributor

Requiring Rails as a runtime dependency causes issues if application does not use ActiveRecord.

@facebook-github-bot
Copy link

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!

@facebook-github-bot
Copy link

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

@danchapman
Copy link

👍

@rmosolgo rmosolgo mentioned this pull request Jan 22, 2016
17 tasks
@vipulnsward
Copy link
Contributor

Doesn't this have issues with sprockets not being available?

@rmosolgo
Copy link
Member

rmosolgo commented Feb 2, 2016

I think our usage of sprockets can be avoided if you're not on rails. Most of it is through Rails.application.assets.

Here's the only place we access Sprockets by name:

sprockets_env = app.assets || Sprockets # Sprockets 3.x expects this in a different place
if Gem::Version.new(Sprockets::VERSION) >= Gem::Version.new("3.0.0")

But I guess if you're not using Rails, you don't ever run that initializer?

@sumskyi
Copy link

sumskyi commented Jun 3, 2016

any chance this will be merged?

I have an app without active_record, arel, mailer, active_job

also, is coffee-script-source needed? probably it can be moved to the development dependencies?

@rmosolgo
Copy link
Member

rmosolgo commented Jun 3, 2016

I would love to merge this PR, but I'm not quite clear how the Sprockets dependency should be handled:

  • Is it always required?
  • Is it only required when Rails is present? (Since the usage is in a Railtie?)

I think a good way to answer this question is either:

  • make a demo app with instructions on how to run & ensure react-rails is working properly
  • add an Appraisal without Rails, or somehow add unit tests in a non-rails environment

As for coffee-script-source, perhaps we could remove it, initially it was added because of a coffee-script bug #168

@sumskyi
Copy link

sumskyi commented Jun 3, 2016

I think, the only dependencies should be there are:

  • railties (ensure it is a rails app)
  • AND
  • sprockets (react-rails will not work without that)

I don't think react-rails should support non-rails environments at all, but I am talking about pretty cut rails app.

I will create a test app than.

Thanks!

@rmosolgo
Copy link
Member

@natemontgomery Thanks for your work on this! I pulled your commits into #558 along with another PR so that I could test the Sprockets dependency.

@rmosolgo rmosolgo closed this Jun 27, 2016
@ghost ghost added the CLA signed label Jul 12, 2016
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.

6 participants