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

#119 config.include and config.exclude does not work correctly with rails engine. #120

Merged
merged 3 commits into from
Jul 16, 2014

Conversation

v1nayv
Copy link
Contributor

@v1nayv v1nayv commented Jul 10, 2014

Fixes #119

@le0pard
Copy link
Member

le0pard commented Jul 10, 2014

Thanks. Need review by @bogdan. But I prefere "_" between engine name and routes :)

@v1nayv
Copy link
Contributor Author

v1nayv commented Jul 10, 2014

Sure i can change it. Let see what @bogdan has to say for this.

@bogdan
Copy link
Collaborator

bogdan commented Jul 15, 2014

I don't think it is right to concatenate routes without _. I've read the discussion at #119 and didn't figure out why did you came to this idea. . is not valid function name indeed. But it don't have to be function name.

Maybe the rightest thing to be done is generate routes for Raiils sub-appliations in JS sub-object:

Routes.engine.root_path
// instead of 
Routes.engine_root_path

Anyway behaviour change of engine route generation is a terrible backward compatibility problem so we need to allow people to have old behaviour.

So in short term I would consider adding underscore here:
https://github.com/railsware/js-routes/blob/master/lib/js_routes.rb#L149

In long term:

  1. add an option how to generate routes for engines:
    • concatenated with '_' (default)
    • nested object
  2. Add a warning that people should specify one and default will be changed in future version ( only if they use engines)
  3. Change the deafult option to the nested object

@v1nayv
Copy link
Contributor Author

v1nayv commented Jul 15, 2014

Sounds good @bogdan . Do you want to do the long term solution on a separate issue or do you want to do it on this one ?

@bogdan
Copy link
Collaborator

bogdan commented Jul 16, 2014

I prefer to go one by one. Rough solution first as we need to support it for pretty long time (maybe even forever).

@bogdan
Copy link
Collaborator

bogdan commented Jul 16, 2014

It is already here: https://github.com/railsware/js-routes#very-advanced-setup

You can submit a PR if you think it is not clear enough.

@v1nayv
Copy link
Contributor Author

v1nayv commented Jul 16, 2014

@bogdan i updated the pull request. Please review

@bogdan
Copy link
Collaborator

bogdan commented Jul 16, 2014

Good job thanks.

bogdan added a commit that referenced this pull request Jul 16, 2014
#119 config.include and config.exclude does not work correctly with rails engine.
@bogdan bogdan merged commit 2f83cc3 into railsware:master Jul 16, 2014
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.

config.include and config.exclude does not work correctly with rails engine.
3 participants