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

Allow defining an array of namespaces to try #439

Closed
spawnia opened this issue Nov 26, 2018 · 2 comments
Closed

Allow defining an array of namespaces to try #439

spawnia opened this issue Nov 26, 2018 · 2 comments
Labels
discussion Requires input from multiple people

Comments

@spawnia
Copy link
Collaborator

spawnia commented Nov 26, 2018

@sadnub has expressed interest in this feature.

It might be useful/necessary for some users to have classes that belong to the same group of functionality (e.g. Models, Queries) split across multiple namespaces.
The current default namespaces allow defining only one namespace though.

Let's discuss if multiple default namespaces are common enough to warrant the extra complexity and if we want to implement them, how we should go about it.

Pro's

  • Convenient short definitions
  • Enables grouping files by domain instead of function

Con's

  • Extra complexity in the implementation without actually adding new functionality
  • Possible source of bugs/ambiguity when you have the same class in different namespaces
  • Goes against the Laravel-like default of grouping files by function, e.g. all Models in one directory

Points to discuss

  • Should this be possible only for Models or for other default namespaces too?
  • Can this only be done in lighthouse.php or is it also possible through @group?
@spawnia spawnia added the discussion Requires input from multiple people label Nov 26, 2018
@yaquawa
Copy link
Contributor

yaquawa commented Nov 27, 2018

Just some ideas.

If the resolving process where can be really complex, users can rebind some of the classes that are used for resolving the namespace in Lighthouse, or just redefine the default directive (can be done with #441)

But, basically let user define the namespace option as an array is the fastest way to achieve their purpose (at their own risk).

@sadnub
Copy link
Contributor

sadnub commented Nov 29, 2018

When I was trying to get this to work, I found that the code base only queries for the default model namespace in two methods. In each of those methods, it puts [ ] around namespaces value and tries to resolve the model in multiple namespaces.

See https://github.com/nuwave/lighthouse/blob/master/src/Schema/Directives/BaseDirective.php

Then look at methods getModelClass and namespaceClassName

This also holds true in the Pagnate directive. The same thing happens.

So my thought was to check if an array was already being passed from the configuration and if not turn it into an array.

See my PR #434

I have been using this in my own app and it is working pretty well. I have over 60 models and my code isn't using the default Laravel directory structure so something like this is a must.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Requires input from multiple people
Projects
None yet
Development

No branches or pull requests

3 participants