Automatically reset static caches in Type, Introspection and Directive when overriding standard types
#1632
Conversation
|
CI is broken because of Renovate. See #1630 (comment) |
ruudk
changed the title
Type::reset and Directive::reset methodsreset method to Type, Directive and Introspection
ruudk
changed the title
reset method to Type, Directive and Introspectionreset method to Type, Directive and Introspection + Store built-in types in static property + Store introspection field definitions as property on instance
ruudk
force-pushed
the
reset
branch
3 times, most recently
from
fa18528
to
457b23c
Compare
|
@spawnia Added a test that covers all 3 reset methods, since they are related. If you clear the Type's cache, the other's also have to be reset to prevent duplicate type instances. |
|
Another idea could be to introduce a single TypeCache object, where all the types are cached. Then there is only 1 place to reset the cache. |
See webonyx#1424 This library uses a static cache for standard types and the internal directives. While not ideal, this works fine. As long as you don't have a scenario where you want to change them. In my situation, we have 2 schema's with a custom ID type. They are not the same instance. This still works fine, but as soon as we run our whole test suite at once, the static cache is initialized in some test, and another test later expects something else. While I think the better solution would be to remove these static caches completely, and store them on an instance instead of statically. But that will be a much bigger task given the current architecture. With these 2 reset methods, we can manually reset the caches in our test suite. It could also be used for people that run their GraphQL server in tools like RoadRunner and FrankenPHP.
This way, it behaves the same as the standard types.
Since the ReferenceExecutor is an instance, we can keep this cache on the instance. Great!
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
I like the idea of centralizing this into a single method that library users are expected to call. From the perspective of a user of this library, isn't the need to reset anything tied to using |
|
Smart! I'll test if that is sufficient. |
|
Alright. When I do it like this, it works perfectly for my use case. But there is 1 test that fails: I think this test does not make much sense to be honest. When you are overriding the standard types, why would you expect the other standard types to be the same instance? Is it expected that the standard types are already initialized (used) before calling overrideStandardTypes? I don't think so. What do you think @spawnia ? |
I think end users don't have to care about this, they can use the overrideStandardTypes method for this.
ruudk
changed the title
reset method to Type, Directive and Introspection + Store built-in types in static property + Store introspection field definitions as property on instanceType, Introspection and Directive when overriding standard types
This reverts commit 386c6dd.
|
I modified the test to be more explicit about what it is trying to achieve and also make it more understandable why we would want that. @ruudk Are you ok with the current state? |
|
@spawnia Yeah this makes sense. I tested it on our CI and it works 🎉 Thanks for the help! |
|
Thank you @ruudk, released with https://github.com/webonyx/graphql-php/releases/tag/v15.18.1. |
See #1424
This library uses a static cache for standard types and the internal directives.
While not ideal, this works fine. As long as you don't have a scenario where you want to change them.
In my situation, we have 2 schema's with a custom ID type. They are not the same instance. This still works fine, but as soon as we run our whole test suite at once, the static cache is initialized in some test, and another test later expects something else.
I think the better solution would be to remove these static caches completely, and store them on an instance instead of statically. But that will be a much bigger task given the current architecture.
When you use
overrideStandardTypes, the static caches will automatically be reset.It could also be used for people that run their GraphQL server in tools like RoadRunner and FrankenPHP.