Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Project separation into multiple dependencies #560

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

Closed
SerafimArts opened this issue Oct 22, 2019 · 15 comments
Closed

Project separation into multiple dependencies #560

SerafimArts opened this issue Oct 22, 2019 · 15 comments

Comments

@SerafimArts
Copy link

SerafimArts commented Oct 22, 2019

Would be good to make the type system a separate dependency so that it can be reused.

The type system should be turned into DTO and isolated from logic (like parsing and serialization).

Reason

The latest ~0.13 version lags far behind the actual code interfaces.
Isolation of the type system and getting rid of logic will make it easier to maintain the current state of the project.

In addition to all this, it will allow to integrate turnkey solutions using webonyx into third-party projects, such as: youshido, railt, digiaonline, etc.

Resume

In the future, it would be nice to separate the type system compiler (SDL) and the executor (GraphQL) into separate repositories (implement a monorepository).

I understand that this issue is rather complicated, but it will allow us to get rid of ecosystem segmentation when each third-party project implements something different, because the rest of the functionality is not required.

@SerafimArts SerafimArts changed the title Package low coupling Project separation into multiple dependencies Oct 22, 2019
@vladar
Copy link
Member

vladar commented Oct 23, 2019

Thanks for bringing this up. Your suggestion makes sense in theory, but I don't see how it will make things easier to maintain. As for me, it is quite the opposite. I guess this is one of the reasons why graphql-js is still a monolithic repo.

There were some discussions about similar things in the past, but they never moved forward.

I guess it will become possible only if someone expresses a will to work on this and maintain this new package in the foreseeable future.

Anyway, I would be happy to hear feedback on this from the community before making any decisions.

@SerafimArts
Copy link
Author

SerafimArts commented Oct 23, 2019

There were some discussions about similar things in the past, but they never moved forward.

Regarding the discussion of AST adoption, this is a moot point. For example, I use my own AST, which supports the extended syntax of the type system and is not compatible with the original. Also has the ability to extend the syntax using extensions (like this).

In any case, I am not ready to discuss this issue now. =)

In this case, my proposal concerns only type system, like intermidiate representation. This will allow us to split the parser itself (compilers frontend) from the executable environment (backend of compilers).

I guess it will become possible only if someone expresses a will to work on this and maintain this new package in the foreseeable future.

In general, I am ready to do this job for webonyx, because made similar actions and only then I thought that it would make sense not to write it)))

I think it’s worth asking the authors (@crisu83 @Jalle19 @viniychuk @portey) of other solutions if they are ready to switch to a single type system, like PSR, and in what form this option will suit them.

@SerafimArts
Copy link
Author

SerafimArts commented Oct 23, 2019

Your suggestion makes sense in theory, but I don't see how it will make things easier to maintain.

P.S. Well, at least ordinary DTO are simpler than classes with logic, like this: https://github.com/webonyx/graphql-php/blob/master/src/Type/Definition/StringType.php#L36-L85

...аnd if it’s not, why does webonyx still not support the option of repeatable directives?

directive @example(name: String!) repeatable on OBJECT

Or why this interface is not deleted, which is no longer in the reference?
;)

@vladar
Copy link
Member

vladar commented Oct 24, 2019

...аnd if it’s not, why does webonyx still not support the option of repeatable directives?
Or why this interface is not deleted, which is no longer in the reference?

Hey, just a friendly reminder that this is an open-source project. Nothing stops you from posting an issue or better a PR mentioning problems you face with. Yet I don't see those in this repo. So I guess that's why those artifacts are still there - nobody complained or mentioned them.

@Jalle19
Copy link

Jalle19 commented Oct 24, 2019

I'm all for trying to reuse code between different implementations.

@SerafimArts
Copy link
Author

SerafimArts commented Oct 24, 2019

Hey, just a friendly reminder that this is an open-source project.

It was a rhetorical question)) Just a hint that with code support, not everything is as good as we would like.


Well, if there are no objections in general, then it is worth determining how to do this.

  1. I can create a repository at home, like serafim/graphql-type-system, where I can create all relevant types in GraphQL\Type\xxx namespace. Implementation tasks are to provide abstractions with the ability to expand them from any project that wants to implement something of its own and take advantage of the ready webonyx ecosystem.
    1.1) Perhaps it makes sense to implement everything with interfaces, for example:
interface ObjectTypeInterface extends TypeInterface, CompositeTypeInterface, ...etc
{
    public function getName(): string;
    public function getDescription(): ?string;
    public function getFields(): iterable; 
    // ...etc
}

1.2) Is it worth implementing abstract classes in this case to simplify implementations?

abstract class AbstractObjectType implements ObjectTypeInterface
{
    private $name;
    public function getName(): string
    {
        assert(is_string($this->name), 'Invariant violation: Name should be defined');

        return $this->name;
    }

    // ...etc
}

1.3) Or immediately implement basic types without interfaces?

class ObjectType implements TypeInterface, CompositeTypeInterface, ...etc
{
    private $name;
    public function getName(): string
    {
        assert(is_string($this->name), 'Invariant violation: Name should be defined');

        return $this->name;
    }

    // ...etc
}
  1. Then prepare a PR that adapts webonyx to use this package.
  2. Then, if there is no objection, I transfer the repository to webonyx/xxxx

Here is a plan of my work. Any objections?

@SerafimArts
Copy link
Author

P.S. I like the implementation from digiaonline. I think that it can be slightly cleaned and used almost in the form in which it is implemented =)

@vladar
Copy link
Member

vladar commented Oct 25, 2019

Please do not rush with it. There is at least one blocker that might make things way more complicated in the future if we choose this approach. We can allow field types to be defined as thunks (one option) or SDL-like strings.

If we have interfaces for everything this would be a breaking change. Without them, we could at least add thunks without any breaks.

Some background on it.

And in general, over-exposure of interfaces provokes breaking changes when they could have been avoided otherwise (there was another discussion about it here)

@SerafimArts
Copy link
Author

SerafimArts commented Oct 25, 2019

Imho, filling the types should be vendor specific. That is, whether they were transferred in the constructor configuration or implemented independently - this is not important.

...and it makes sense to make only interfaces, like:

interface ObjectTypeInterace ...
{
    public function getFields(): iterable;
}

In this case

Configuration via constructor

class ObjectType implements ObjectTypeInterace
{
    private $fields = [];
    public function __construct(array $config)
    {
        $this->fields = $config['fields'] ?? [];
    }

    public function getFields(): iterable
    {
        return $this->fields;
    }
}

Configuration via SDL

class ObjectType implements ObjectTypeInterace
{
    // ...

    public function getFields(): iterable
    {
        $parse = fn (string $field) => $this->parser->parse($field);

        return \array_map($parse, $this->fields);
    }
}

Custom Types

class UserType implements ObjectTypeInterace
{
    // ...

    public function getFields(): iterable
    {
        yield 'id' => new Field(new IDType());
        yield 'name' => new Field(new NonNullType(new StringType()));
    }
}

I don’t see such tasks yet, when the interface becomes incompatible.

That's why I said that I like the implementation of digia, because it allows you to not depend on implementations, and the interface defines only methods for obtaining data. As example: https://github.com/digiaonline/graphql-php/blob/master/src/Type/Definition/FieldsAwareInterface.php

@SerafimArts
Copy link
Author

SerafimArts commented Oct 25, 2019


Well, in any case, as promised - I sketched the code based on the JS ref version 14.5:

The basic set of classes for implementing types based on configs here:

@vladar @Jalle19 If you do not mind, I will invite you to this group so that everyone can make complaints about the implementation and correct what they don't like.

It is worth considering that this is only a sketch.

@SerafimArts
Copy link
Author

I would like to further discuss some issues. Such types implementation allows you to already implement cross-design extensions that implement additional types.

But in this implementation, they do not contain behavior. That is, handler methods like parse and serialize in scalars.

  • Such types can be represented in any format, for example, in the form of JSON.
  • They can be serialized/cached by the PSR-6/16 without troubleshoots.
  • They are simple and in my opinion, the current implementation allows you to begin work on implementing support in any project.
  • But... This will not allow for more flexible extensions. For example, implement a DateTime scalar with parsing and serialization logic or type Any... Or something else.

Should I try to take into account all the ready-made implementations from digia, railt and webonyx and try to implement independent handler methods or the current version is enough for now?

@vladar
Copy link
Member

vladar commented Nov 2, 2019

I have a gut feeling that this whole splitting would cause more trouble than provide benefits. I guess I need a set of clear and specific examples of what it will give the community (vs abstract ideas about how good it could be). So that we could really do some cost/benefit analysis.

Also, PoC is totally required for this library as we introduce strict PHPStan checks and I feel that it won't be an easy ride (since utils rely on some fields of type objects like astNode or extensionASTNodes which are not exposed by interfaces).

Plus interfaces almost always imply factories for producing specific instances. And injection of those factories. Otherwise, tools like buildSchema or schemaExtender just won't work.

@SerafimArts
Copy link
Author

Yes, I understand.

I think it’s worth the wait when I implement several tools for this set of interfaces, and then it will be possible to say with confidence whether they are needed for webonyx or not.

@vladar
Copy link
Member

vladar commented Nov 2, 2019

Anyways, you didn't mention if you have any specific situation which made you want to have this. There may be different approaches to reach the same goal but it's hard to help without understanding specifics.

@SerafimArts
Copy link
Author

SerafimArts commented Nov 3, 2019

Personally, in my case, as I said, this will simplify the implementation of webonyx executor in my solution. Thus, I will get a ready-made solution for my compiler, where this set of interfaces will act as an implementation of IR (intermediate representation).

Like this:
image

Now I have already generate code that is fully compatible with these interfaces. It remains to finish the little things, such as namespaces, generics and other).

That is, if webonyx supported them, I would not have to do anything to make the solution work, and webonyx would receive support for an alternative SDL compiler, whose lexer, by the way, is twice as fast (see my fork of ur benchmark: https://github.com/SerafimArts/graphql-bench) =)))

And webonyx and digiaonline projects can be obtained, for example:

  1. GraphQL SDL generator
  2. Transpiler from PHP to JS implementation
  3. Scheme-oriented libraries (like semi-automatic integration/unit tests)
  4. Cross-platform (idk how it's right to call) implementations of types. But without runtime methods (parse/serialize/resolve/etc) it’s not so profitable.

This is what I was able to come up with that can be implemented based on this interfaces.

@vladar vladar closed this as completed Mar 25, 2021
@vladar vladar reopened this Mar 25, 2021
@vladar vladar closed this as completed Mar 25, 2021
@vladar vladar reopened this Mar 25, 2021
@vladar vladar closed this as completed Mar 27, 2021
@webonyx webonyx locked and limited conversation to collaborators Mar 27, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

3 participants