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

brittle assumptions about rails environment names #64

Closed
decasia opened this issue Jan 23, 2015 · 10 comments
Closed

brittle assumptions about rails environment names #64

decasia opened this issue Jan 23, 2015 · 10 comments

Comments

@decasia
Copy link
Contributor

decasia commented Jan 23, 2015

As things stand, if you use any non-standard development environment names, there is no good way to have them use the development mode in ember-cli-rails, because ember-cli-rails makes hardcoded assumptions about the names of Rails environments:

https://github.com/rwz/ember-cli-rails/blob/master/lib/ember-cli/helpers.rb#L25
https://github.com/rwz/ember-cli-rails/blob/master/lib/ember-cli/middleware.rb#L18

It seems to me that there any time there are checks like these, there ought to be a config option to override the defaults. @rwz, would you be open to a PR along those lines?

@rwz
Copy link
Collaborator

rwz commented Jan 23, 2015

I have never ever seen people use non-standard name for development environment. I don't see a good reason to do that and I expect every other gem to instantly break since these assumptions are pretty much everywhere.

On the other hand I'm seeing people use custom production-like environments all the time. Staging might be the most popular name choice these days. ember-cli-rails is designed to handle these cases properly and act as it's production.

I don't really see much value in adding additional configuration around this and I don't really imagine a nice clean way of handling it. If you have suggestions regarding how it might look like, feel free to post it here. We might end up implementing them or merging a PR with implementation.

@rwz rwz closed this as completed Jan 23, 2015
@decasia
Copy link
Contributor Author

decasia commented Jan 23, 2015

Well, it seems rather hasty on your part to dismiss my issue out of hand on the argument that you've "never seen someone do this" when I'm here to tell you that there is at least one outfit that does this (mine).

To quote DHH on this one:

To run six environments like we do, you can’t just rely on Rails.env.production? checks scattered all over your code base and plugins. It’s a terrible anti-pattern that’s akin to checking the class of an object for branching, rather than letting it quack like a duck. The solution is to expose configuration points that can be set via the environment configuration files.

So, according to DHH, of all people, what you are doing is a terrible anti-pattern. He also adds: "Rails was built for it from the beginning. The defaults are just a starting point."

@rwz
Copy link
Collaborator

rwz commented Jan 23, 2015

We're not relying on Rails.env.production? anywhere in the codebase.

@decasia
Copy link
Contributor Author

decasia commented Jan 23, 2015

@rwz
Copy link
Collaborator

rwz commented Jan 23, 2015

Like I've said, we're not relying on it anywhere. We're using it as one of the marks of non-production environment. Because environment is clearly non-production if it's production.

@decasia
Copy link
Contributor Author

decasia commented Jan 23, 2015

Well, in terms of a low-impact way of providing some form of configurability here, how about something like this:

# helpers.rb
def non_production?
  Rails.configuration.enable_ember_middleware ||
  (!Rails.env.production? && Rails.configuration.consider_all_requests_local)
end

def live_recompilation?
  Rails.configuration.use_ember_live_recompilation || Rails.env.development?
end

# middleware.rb#enable_ember_cli
- if Rails.env.development?
+ if Helpers.live_recompilation?

This would leverage the Rails config architecture that you're already depending on, preserve the defaults that you already have set up, and make it trivial for gem users to override as necessary.

@rwz
Copy link
Collaborator

rwz commented Jan 23, 2015

LGTM. Want to send a PR?

@rwz rwz reopened this Jan 23, 2015
@decasia
Copy link
Contributor Author

decasia commented Jan 23, 2015

Sure. Thanks!

@rwz
Copy link
Collaborator

rwz commented Jan 25, 2015

Closed by #65

@rwz rwz closed this as completed Jan 25, 2015
@rondale-sc
Copy link
Collaborator

👍

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

No branches or pull requests

3 participants