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

Config route #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Config route #26

wants to merge 2 commits into from

Conversation

mohawk2
Copy link
Contributor

@mohawk2 mohawk2 commented Nov 7, 2018

Allows separate configuration of the yancy_path and the api_path - latter now attaches itself to the parent of the editor route, not to the editor route itself. This allows e.g. /editor and /api, not just /yancy and /yancy/api.

I wanted to make the static files (bootstrap.css et al) to be served from the same URL location, e.g. /editor/bootstrap.css, but I couldn't figure out how. Maybe something to do with "static", or "assets"?

@mohawk2
Copy link
Contributor Author

mohawk2 commented Nov 8, 2018

I had to change the API route back to a child of the editor route, since separating them completely caused complications for the Auth::Basic plugin.

@preaction
Copy link
Owner

Yeah, because of that auth problem, we might have to hold off on this until the editor is less-integrated and more standalone. Then the auth plugins will have to adapt.

Instead of paths, I would prefer route objects (Mojolicious::Router::Route, obtained via $app->router->{any,get,post,put,delete,patch,under}( $path )). For ease-of-use, the plugin/standalone app can detect a string passed to a route config and turn it in to a route object.

So the configuration I'd like to see would be editor => { route => '/yancy', api_route => '/yancy/api' }. Then make sure they have knowable, unique names that can be looked up. Then the Auth API should be able to find them.

I'm sure there's things I'm not getting and we'll have to hammer them out... If we're not careful, what order we declare routes in can end up hiding routes altogether...

@mohawk2
Copy link
Contributor Author

mohawk2 commented Nov 8, 2018

I will revisit this in a bit. In the meantime, what do you think of the other PR? If you like and could merge it, that reduces what I call the "overhang" of things that might clash with each other :-)

@preaction
Copy link
Owner

Sorry, done, yes. And as expected this now conflicts, which is fine.

@mohawk2 mohawk2 force-pushed the config-route branch 3 times, most recently from 48e537b to 610abe3 Compare November 15, 2018 04:46
@mohawk2
Copy link
Contributor Author

mohawk2 commented Nov 15, 2018

I've now switched it to use editor => { ... } as you specified above. The editor.api_route is still a child of the editor.route. Next I'll be looking at how to make this work with the Auth plugin.

One big conceptual question which I'm not sure about yet: how to have more than one API thingy, while still allowing the "no-code" concept I'm keen on. I gather you're after having both an admin UI, with its pet API, while also having a "proper" app.

Would this be managed with the main app having two Mojolicious::Plugin::Yancy on the app, one for admin purposes, one for "main app" purposes? That would allow "an" MPY to have "its" editor (if any), and API. This could then be distinguished within a wider app, by having a namespace passed in to the MPY, under which its helpers would be placed, default "yancy". If it's this, then the standalone app could just have some means of specifying this arrangement, allowing config of one MPY to be an admin UI, and the other to be just an API from a given OpenAPI spec.

What do you think?

@mohawk2
Copy link
Contributor Author

mohawk2 commented Nov 15, 2018

To be clear, the "this" to work with the Auth would be to allow the editor.api_route to be anything rather than only a child of the editor.route, and for the Auth to have a way to find both editor and api routes and attach to them.

@preaction
Copy link
Owner

I'm cool with having the ability to have multiple Mojolicious::Plugin::Yancy, and namespace config would be a good way to do that, but that's not needed for this.

Remember, the editor is inside the Mojolicious::Plugin::Yancy. The API for the editor is built automatically by Mojolicious::Plugin::OpenAPI, we just need to add x-mojo-to to the OpenAPI spec we get. The editor.api_route is what we give to Mojolicious::Plugin::OpenAPI to put the routes it creates, and the editor.route is where the editor application is served.

The proper app is everything the user does that isn't the Yancy plugin.

The auth plugin could just ask the editor what routes it has (add two helpers, yancy.editor.route and yancy.editor.api_route). The yancy.route helper was built specifically for the auth plugin to get at the editor routes. Now that Yancy is a bit more than just an editor, it makes sense to change this: Deprecate the yancy.route helper in favor of the yancy.editor.route and yancy.editor.api_route helpers.

@mohawk2 mohawk2 force-pushed the config-route branch 2 times, most recently from e88f7a9 to 7badc39 Compare November 21, 2018 09:19
@coveralls
Copy link

coveralls commented Nov 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling ba07b50 on mohawk2:config-route into 28d1cb1 on preaction:master.

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.

3 participants