Pass Rails env as factory

[chytreg]

Read the diff :)

[jandudulski]

Love the idea, but why using helper and injecting js into dom instead of separate js file? e.g.

# app/assets/config.js.coffee.erb
angular.module('app').service 'Config', ->
  {
    foo: '<%= Figaro.env.foo %>'
  }

And also, as you can notice, I think we should use service instead of factory and services/factories should start with capital letter.

[chytreg]

coffe.erb bad idea, problems with asset caching :(

[chytreg]

Btw what is wrong with solutions like https://github.com/gazay/gon?

[jandudulski]

This article says I was wrong - we shouldn't use service here. But it also shows a better thing than factory: app.constant('envConfig', {foo: 'bar' }) seems perfect.

About gon and similar - I just don't like any js and css inside document, but if erb sucks we probably don't have a choice :(

[teamon]

gon is global, here we are creating proper angular dependency. constatn indeed seems perfect :)

[chytreg]

Could we merge or [anyone - @sheerun] against?

[sheerun]

I'd change constant name to ENV

[sheerun]

Hash should be in ApplicationController, and JS code in view btw.

[teamon]

Why?

Tymon Tobolski

On Wednesday, August 21, 2013 at 12:47 PM, Dariusz Gertych wrote:

Read the diff :) You can merge this Pull Request by running git pull https://github.com/monterail/guidelines feature/js-env Or view, comment on, or merge it at: https://github.com/monterail/guidelines/pull/178 Commit Summary Pass Rails env as factory

File Changes M javascript.md (https://github.com/monterail/guidelines/pull/178/files#diff-0) (37)

Patch Links: https://github.com/monterail/guidelines/pull/178.patch https://github.com/monterail/guidelines/pull/178.diff

[sheerun]

What why

[teamon]

Why should it be put in ApplicationController and js in view?

Tymon Tobolski

On Wednesday, August 21, 2013 at 12:47 PM, Dariusz Gertych wrote:

Read the diff :) You can merge this Pull Request by running git pull https://github.com/monterail/guidelines feature/js-env Or view, comment on, or merge it at: https://github.com/monterail/guidelines/pull/178 Commit Summary Pass Rails env as factory

File Changes M javascript.md (https://github.com/monterail/guidelines/pull/178/files#diff-0) (37)

Patch Links: https://github.com/monterail/guidelines/pull/178.patch https://github.com/monterail/guidelines/pull/178.diff

[sheerun]

Because you may want to pass settings from request or some controller method that is not exposed as helper. Helpers are just for rendering, not for fetching data from Figaro.env.

[sheerun]

If you really want helper, create one that accepts hash of attributes to pass to angular.

[teamon]

If you need to pass such data there is something wrong with app architecture. This should be constant for application and stuff like current_user.id etc are already available as helper methods and accessible in application helper.

Tymon Tobolski

On Wednesday, August 21, 2013 at 12:47 PM, Dariusz Gertych wrote:

Read the diff :) You can merge this Pull Request by running git pull https://github.com/monterail/guidelines feature/js-env Or view, comment on, or merge it at: https://github.com/monterail/guidelines/pull/178 Commit Summary Pass Rails env as factory

File Changes M javascript.md (https://github.com/monterail/guidelines/pull/178/files#diff-0) (37)

Patch Links: https://github.com/monterail/guidelines/pull/178.patch https://github.com/monterail/guidelines/pull/178.diff

[sheerun]

I'm against merging this code.

[chytreg]

It was said: [anyone - @sheerun] :P

Btw @sheerun now you are trying to invent gon for Angular.

Maybe we should write our on include_gon helper. What do you think?

Best regards Dariusz Gertych

2013/8/21 Adam Stankiewicz notifications@github.com

I'm against merging this code.

— Reply to this email directly or view it on GitHubhttps://github.com/monterail/guidelines/pull/178#issuecomment-23012580 .

[teamon]

This is 5 lines of code and you can write whole gon in another 5 (+ rewriting half of it to replace include_gon helper). For me current solution in this PR is perfect.

Tymon Tobolski

On Wednesday, August 21, 2013 at 12:47 PM, Dariusz Gertych wrote:

Read the diff :) You can merge this Pull Request by running git pull https://github.com/monterail/guidelines feature/js-env Or view, comment on, or merge it at: https://github.com/monterail/guidelines/pull/178 Commit Summary Pass Rails env as factory

File Changes M javascript.md (https://github.com/monterail/guidelines/pull/178/files#diff-0) (37)

Patch Links: https://github.com/monterail/guidelines/pull/178.patch https://github.com/monterail/guidelines/pull/178.diff

[sheerun]

1. Gon is more advanced, and it's irrevelant to the issue I'm pointing
2. Fetching data from helper is wrong

[teamon]

Ad 2. So lazy loaded current_user helper is also wrong?

Tymon Tobolski

On Wednesday, August 21, 2013 at 12:47 PM, Dariusz Gertych wrote:

Read the diff :) You can merge this Pull Request by running git pull https://github.com/monterail/guidelines feature/js-env Or view, comment on, or merge it at: https://github.com/monterail/guidelines/pull/178 Commit Summary Pass Rails env as factory

File Changes M javascript.md (https://github.com/monterail/guidelines/pull/178/files#diff-0) (37)

Patch Links: https://github.com/monterail/guidelines/pull/178.patch https://github.com/monterail/guidelines/pull/178.diff

[chytreg]

Ad 1) It's not irrelevant because is working like you want to work, (e.g. push data from controller) Ad 2) Do you think more clear way is to code it directly in the view?

Best regards Dariusz Gertych

2013/8/21 Adam Stankiewicz notifications@github.com

1. Gon is more advanced, and it's irrevelant to the issue I'm pointing
2. Fetching data from helper is wrong

— Reply to this email directly or view it on GitHubhttps://github.com/monterail/guidelines/pull/178#issuecomment-23012970 .

[sheerun]

Ad 1. It's irrelevant. Fetching data from helper is universally wrong. Ad 2. Helper is last place I would check for variables that are passed to angular. CONTROLLER is the place to fetch data, and pass data to the VIEW. VIEW may pass data to HELPER (not necessary).

[chytreg]

Could you propose more valid version?

[jandudulski]

class ApplicationController < ActionController::Base
  def js_env
    {
      foo: 'bar'
    }
  end
  helper_method :js_env
end

[chytreg]

:baby_bottle:

[chytreg]

@Ostrzy any comment?

[jandudulski]

yyy, sorry:

def js_env_data
    {
      foo: 'bar'
    }
  end
  helper_method :js_env_data

And helper will just print this data.

[sheerun]

I guess the same helper defined in controller + helper_method is ok. You may even render in controller, I don't care. Just don't fetch data directly from application_helper.rb

[chytreg]

Here in the office we can't agree with that solution. I know is more valid, but we do not see the use case of this solution. Why we should have the method in controller that we never use? The assumption is that the js_env is not extendable the same as figaro application.yml config.

[sheerun]

You may use before_filter and @jsData if you don't want method you won't use.

[teamon]

@jsData is even worse. I'd rather have single place (helper) with everything than such simple functionality splitted into 3 (or more) files.

[sheerun]

It should be fix: Make @sheerun happy ;>

[teamon]

... or we should wait for @szajbus opinion.

[szajbus]

I am way behind you guys on angular stuff...

[chytreg]

@szajbus The problem is not about angular, but where to put rails code. Unfortunately must read some previous comment to get what is going on :smile:

[szajbus]

@chytreg That's what I feared! :)