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

Override Orchid Routes in Apps #16

Closed
jduss4 opened this issue May 15, 2017 · 13 comments
Closed

Override Orchid Routes in Apps #16

jduss4 opened this issue May 15, 2017 · 13 comments

Comments

@jduss4
Copy link
Contributor

jduss4 commented May 15, 2017

Right now I don't know how to override or alter routes that were drawn by the orchid engine. I haven't read too much about it, yet, but spitballing ideas

  • copy / inject routes into app routes.rb so easily edited there
  • figure out some fancy way of manipulating the routes creation (prepend? append?)
  • put it under the orchid namespace and then see if it gets called correctly based on location?

http://edgeguides.rubyonrails.org/engines.html#routes

@techgique
Copy link
Member

From Jess: "... you can't override a named route. Like item_path is going to always go to items#show at /items/:id because we can't override it to be /:id or whatever the WCA needs"

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 17, 2017

This thread is for rails 3, but might have something we can try out: https://stackoverflow.com/questions/5997527/overriding-named-routes-provided-by-rails-3-engines

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 18, 2017

Okay, it looks like the app's routes load before the generator's routes load. This means that any gnarly code can be done on the engine's side and most casual users won't need to know about it.

There is likely a better way to do this, but I'm thinking...what if we run each of orchid's routes through a check. If there isn't currently a route with that name, we add it. If there is, we skip the orchid version (maybe outputting a warning about it being overridden?).

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 18, 2017

So, using the above idea about only loading orchid routes if no similarly named app route, I mocked up a branch:

47acaaf

This does appear to work in general cases. The bad news is that Cather specifically wants the letters at /letters/let0183 which currently means the root of the rails app. This means that if we add the route get /:id, it's going to match on EVERYTHING. We ideally need it to be loaded last. My mockup branch wouldn't address that. I think we could probably use "match" instead of "get" to make sure that only urls matching letter ids would be caught in that first overridden rule in the app routes?

http://guides.rubyonrails.org/routing.html#advanced-constraints

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 19, 2017

I believe that this now addresses the above concern, also. My questions now are:

  • is there a better way to accomplish this?
  • does it make sense to us the developers?

https://github.com/CDRH/orchid/tree/route_test

@techgique
Copy link
Member

Code is reviewed and ready, but Jess said she'll move documentation to README.

We had discussed it before and I can't remember if we came to any conclusion. Do we want to add further constraints on the :id value? I recall not thinking it a good idea because one can't add anchors, but I read that all the constraints are anchored to the beginning of the string (http://guides.rubyonrails.org/routing.html#segment-constraints) so that's no longer a reason to dismiss them.
I think we were just hoping to avoid all the unnecessary API calls and return 404s for bad IDs. Are there any or only a handfull of non-#### documents for it to retrieve?

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 25, 2017

I think I would prefer that the item path's id stays fairly open in orchid, but we could lock it down by app / project as needed?

@techgique
Copy link
Member

Excellent point. I agree.

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 27, 2017

Can this issue be closed?

@techgique
Copy link
Member

After I add documentation to the PR and we merge it :)

@jduss4
Copy link
Contributor Author

jduss4 commented Oct 27, 2017

Oh! I thought all the routes stuff had already been merged, my bad

@techgique
Copy link
Member

techgique commented Oct 30, 2017

Oh, sorry I misread which issue this was when I posted that. There is the routing with Passenger deployment bug I'm working on now though, so that's a reason to leave it open. Will close this once that's sorted out.

@techgique
Copy link
Member

Orchid routes don't work when the main app is deployed via Phusion Passenger because of how it spawns processes. It must be set to the 'direct' SpawnMethod for the routes to be present in the app. This can only be set vhost-wide and we don't want to do that for all future Orchid-based Rails apps. Looking into how to modify the code to be compatible.

techgique added a commit that referenced this issue Nov 3, 2017
Previous approach with `require_relative`, aside from feeling very hacky
and not the Rails way, breaks when deployed via Phusion Passenger
because of its smart spawning method to save resources
and speed up extra app process creation:
https://www.phusionpassenger.com/library/indepth/ruby/spawn_methods/

Symlink `config/routes.rb` from `config/initializers/` to properly load
`ROUTES` constant before main app's routing is processed

Remove class variables as they don't work well with Passenger smart
spawning either. Instead:
- Only call draw method from `routes.rb` if `ROUTES` defined
- Directly check drawn routes to determine whether Orchid should
draw its routes when draw method called

Check whether home path drawn, as apps will always need a home path
and its location should never change, though its controller and view
will often be overridden in the main app

Remove `scope url_prefix` which breaks routes when deployed
via Passenger, our preferred method of deploying Rails apps at sub-URIs

Change `Orchid::Routing` to a module since it is never instantiated

Use `module_function` scoping for draw method per style guide:
https://github.com/bbatsov/ruby-style-guide#module-function

Close #16
@jduss4 jduss4 closed this as completed in #56 Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants