nnjeim / world

A Laravel package which provides a list of the countries, states, cities, currencies, timezones and languages.
MIT License
761 stars 104 forks source link

Make routes optional #21

Closed emiliopedrollo closed 2 years ago

emiliopedrollo commented 2 years ago

Honestly I don't think it makes sense on a package like that to define routes. Maybe I'm just misunderstanding what this project is supposed to be.

Relates to #16.

nnjeim commented 2 years ago

@emiliopedrollo Hi and thank you for your contribution. The package can be used as an api or consumed via the world facade. This is the reason why the routes are defined. I will pull this route and test your proposed modification. Thank you

nnjeim commented 2 years ago

@emiliopedrollo in world.php config files you can find the seeders key:

    'seeders' => [
        'states' => true,
        'cities' => true,
        'timezones' => true,
        'currencies' => true,
        'languages' => true,
    ],

What do you think if routes and seeder keys are combined into :

'modules' => [
                 'states'  => true,
        'cities' => true,
        'timezones' => true,
        'currencies' => true,
        'languages' => true,
],

If you agree with this. the seeder action and the routes files should be modified accordingly. If you would like to keep the control over using the module as an api or not you can add the entry below:

'routes' => true,
emiliopedrollo commented 2 years ago

That also makes sense but it's less control to the user of the package, in general I think it's a good idea to provide options with modularity instead of monolitic approaches.

I think we can also look at some official Laravel packages for inspiration like laravel/passport where a RouteRegistrar is used. By default it could register all the routes but the user could override it on a ServiceProvider if he/she so chooses:

Passport::routes(function (RouteRegistrar $router) {
    $router->forAccessTokens();
    $router->forPersonalAccessTokens();
    $router->forTransientTokens();
});
nnjeim commented 2 years ago

@emiliopedrollo Thank you for this. I would opt to keep things simple in a config file if you agree with this approach.

nnjeim commented 2 years ago

@emiliopedrollo thank you, i am merging the PR.

kodjosama commented 2 years ago

@emiliopedrollo Thanks dude, that's awesome. I was hoping to implement this feature when I got time. Thanks, @nnjeim for this package.

nnjeim commented 2 years ago

@kodjosama Thank you. All the credit goes to @emiliopedrollo