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

[2.x] enh: Add Twill Route facade to preserve singleton() method #1964

Closed
wants to merge 1 commit into from
Closed

[2.x] enh: Add Twill Route facade to preserve singleton() method #1964

wants to merge 1 commit into from

Conversation

pboivin
Copy link
Contributor

@pboivin pboivin commented Dec 1, 2022

This PR adds a Route facade in Twill. The facade is equivalent to Illuminate\Support\Facades\Route, and adds a singleton() static method as an alias for the twillSingleton() macro (renamed in 2.12.0).

This offers an alternative migration path on 2.x, which is to use A17\Twill\Facades\Route within routes/admin.php.

ref: #1961

@pboivin pboivin changed the title enh: Add Twill Route facade to preserve singleton() method [2.x] enh: Add Twill Route facade to preserve singleton() method Dec 1, 2022
Copy link
Member

@ifox ifox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @pboivin, thank you!

@haringsrob
Copy link
Contributor

I feel we should name this TwillRouter instead?

@haringsrob
Copy link
Contributor

haringsrob commented Dec 2, 2022

We also already have the TwillRoutes facade. We could just use that for Twill specific routes?

<?php

use Illuminate\Support\Facades\Route;
use A17\Twill\Facades\TwillRoutes;

Route::get('bar');
Route::singleton('some-singleton');
TwillRoutes::module('pages');
TwillRoutes::singleton('menuLinks');

We can add this to 2.x as well, but in 3.x this should be the default to use our facade.

3.x branch

@pboivin
Copy link
Contributor Author

pboivin commented Dec 2, 2022

Hey @haringsrob, thanks for the feedback! A few thoughts:

  • The scope if this PR is to provide an alternative solution (aka. to solution I wish I had) to preserve the Route::singleton() API. I'll admit I did not yet think about how this can evolve for 3.x.
  • In my view, Route is more aligned with the original Illuminate\Support\Facades\Route. (Come to think of it, Twill's Route could even extend it, to inherit the @method annotations.)
  • I think the Twill prefix here is a bit redundant... it's already in the namespace.
  • At a glance, the existing TwillRoutes class is related, but on the other hand, it's not anything I would want to call in the context of routes/admin.php, as a user. I don't know if it's the best place to mix in the module() and singleton() implementations.
  • Finally, is there a need to use both Route facades in routes/admin.php?

@haringsrob
Copy link
Contributor

Hey,

I believe that with this approach we are overwriting the singleton method provided by Laravel, so we cannot use that and the Twill singleton together. I want to avoid confusion and leave that functionality intact, that is why the prefixing in the current macro was done.

The same goes for why I opt to reuse TwillRoutes instead of a new Routes facade, I know it is already name spaced, but the (when mixed (which I think would not happen much)) would require to make an alias. And here again, this may cause confusion when writing routes as in accidentally taking the wrong facade.

While it does not happen much, I do take from this experience that using to generic macro's may lead to conflicts in the future, and for this we better take things into our own hands to avoid this from happening again.

Sorry if I am misunderstanding, if so, please let me know.

@pboivin
Copy link
Contributor Author

pboivin commented Dec 2, 2022

It makes sense, thanks for the precision.

I guess the main point I'm thinking of is that switching to a A17\Twill\Facades\Route facade preserves the API I know and want. I think you're seeing further ahead and I appreciate that.

@pboivin pboivin closed this Dec 2, 2022
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