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

Update package to remove CLI as dep for client #165

Open
pokryfka opened this issue Oct 11, 2023 · 7 comments
Open

Update package to remove CLI as dep for client #165

pokryfka opened this issue Oct 11, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@pokryfka
Copy link
Contributor

pokryfka commented Oct 11, 2023

SwiftGraphQLCLI is not needed for applications using SwiftGraphQLClient and it brings along many dependencies like Spinner, Files, Yams as well as swift-format and, indirectly, SwiftSyntax which does not follow SemVer and easily causes conflicts (see discussion here).

Please consider moving SwiftGraphQLCLI to its own package.

@shaps80
Copy link
Collaborator

shaps80 commented Oct 19, 2023

Yep agreed! I raised this the other day with maticzav as well and we agree.
I'll likely address this tomorrow or early next week 👍

@shaps80 shaps80 self-assigned this Oct 19, 2023
@shaps80 shaps80 added the enhancement New feature or request label Oct 19, 2023
@shaps80 shaps80 changed the title Move SwiftGraphQLCLI to its own package Update package to remove CLI as dep for client Oct 19, 2023
@shaps80
Copy link
Collaborator

shaps80 commented Oct 27, 2023

Hey all, I unfortunately got sick and didn't manage to get to this. I now have some other priorities so if anyone else sees this in the next week or two and wants to pick it up, feel free to do so thanks! @pokryfka / @maticzav

@shaps80 shaps80 added help wanted Extra attention is needed good first issue Good for newcomers labels Oct 27, 2023
@maticzav
Copy link
Owner

@shaps80 do you know of any Swift project that has two packages in the same repository and we could use as reference to do this?

@shaps80
Copy link
Collaborator

shaps80 commented Oct 30, 2023

Yeah I think I might have one myself. I've looked into this separately and I think I worked out the issue and maybe even resolved it. I'll look into this again later as I'm not working today. My son is home so I won't be able to get much done till tomorrow.

@CrownedPhoenix
Copy link

Before my attempt in this PR (which moves CLI to a separate package) I played around with a sub-package for the CLI as well.

I had some success but the tricky part I ran into was exposing the CLI to the importer of swift-graphql via a plugin.
I was able to get it working but I did some weird stuff where I made a plugin in swift-graphql that looks up the path of the sub-package CLI executable and calls it using Process.
This smells bad to me (which is why I pursued separating the package entirely).
The biggest issue with it was that it required me to disable the sandbox in the plugin since there wasn't any way to allow the execution of another executable from within the build directory (for obvious reasons).

Anyway, just food for thought. Perhaps there's another way to sub-package that doesn't suffer from the same problem.

I suppose that in the current workflow the CLI leans towards beig built and installed on the dev machine rather than run as a package plugin/executable. I definitely prefer the package plugin/executable structure since it's one step simpler but that's just an opinion.

@CrownedPhoenix
Copy link

Happy to take this on if I can get some direction on:

  • Do we want a separate package or sub-package?
  • Any requirements or constraints there are about how the user interacts with the CLI?

@shaps80
Copy link
Collaborator

shaps80 commented Mar 9, 2024

@CrownedPhoenix this is ultimately the goal however considering the large scale changes that are in progress right now for v6, it's premature to attempt this just yet. However once we get more of the core library in a branch where others can test, this is likely a step we'd be keen on. Again, we're not there yet and we do not want to introduce large scale effort into v5 right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: Todo
Development

No branches or pull requests

4 participants