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 lazy enum values definition #862

Closed
riffraff opened this issue Aug 2, 2017 · 6 comments
Closed

Allow lazy enum values definition #862

riffraff opened this issue Aug 2, 2017 · 6 comments

Comments

@riffraff
Copy link
Contributor

riffraff commented Aug 2, 2017

I have an Enum type defined statically:

KindsEnumType =  JobTypeEnumType = GraphQL::EnumType.define do
  Kind::KINDS.each { |name| value name.camelize }
end

and this works fine, but now I want to move KINDS from a ruby constant to a configuration value stored in the database.
This means, I would change the code to

KindsEnumType =  JobTypeEnumType = GraphQL::EnumType.define do
  Kind.public.each { |record| value record.name.camelize }
end

the problem is that in specs this might run before I have initialized the database, causing unstable tests.

If the enum could be defined lazily, this could be solved, e.g. reusing values

KindsEnumType =  JobTypeEnumType = GraphQL::EnumType.define do
  values -> { Kind.public.map { |record| record.name.camelize } }
end

Is this somehow possible?
Would it cause problems?
I can try to make a PR if it's something that makes sense.

@riffraff riffraff changed the title Allow lazy vales for enums Allow lazy enum values definition Aug 2, 2017
@rmosolgo
Copy link
Owner

rmosolgo commented Aug 2, 2017

I think this is possible! But, I think the question is, when would values be called? We need to know all values in order to validate inputs and coerce outputs, but maybe we can delay it until then.

The other issue is thread-safety and fork behavior. If we don't populate values in the parent process, then forked processes will all re-calculate this value each time, right?

I'm definitely open to a PR, sounds like a nice improvement if we can work out the flow.

@riffraff
Copy link
Contributor Author

riffraff commented Aug 4, 2017

Yes, my idea was to delay it until we need to load the values, i.e. validate input or dump the schema.

In a accept/fork/process model, that would mean loading the values once per request in the forked process yes, it seems reasonable to me and allows updating the configuration data between requests.

I am not sure whether we should store the computed value for the duration of the request, or where to store it though.

I mean, if we have something like

def values
 @cached_values ||= compute_values()
end

that might create problems in non-forking applications, but if we don't we might hit the database twice (potentially not a big deal though)

Do we have something in the graphql codebase that would allow us to cache data per-request, (whatever the request lifecycle is)?

@rmosolgo
Copy link
Owner

rmosolgo commented Aug 6, 2017

something in the graphql codebase that would allow us to cache data per-request

there's not something per-request, but context is good per-query storage.

I'm torn because schema filters allow hiding some parts of the schema. But I can't think of any way to add parts of the schema or determine the schema dynamically :S :S

Actually, I'm starting to work on a new API for schema definition. I have some thoughts like using a type_missing hook to support dynamic type definition and field_missing to support dynamic field resolution. Maybe this will offer some option, I'll let you know when I push it :)

@rmosolgo
Copy link
Owner

Just sharing another thought on this topic:

Currently, a Schema is a fixed set of types and fields. What if it wasn't fixed, and instead, something like schema filtering was added that could return anything. So, you could add and remove types to the schema dynamically. This is a bit like refactoring every class in your Ruby app to use only method_missing and no static method definitions -- so that while the app is running, the set of implemented methods may change. Very weird but it might work in this case, and it would also support things like:

  • Extending the introspection types
  • Custom dynamic fields (user-defined fields which behave like __typename)

@vergenzt
Copy link

vergenzt commented Oct 31, 2017

Just found this thread as I was googling for dynamic enum values in another language's GraphQL lib, but thought I'd chime in with caution against going too far down the dynamic rabbithole. I could see it being useful for certain (rare) edge cases, but otherwise I think you lose a lot of the client-side value of GraphQL if your schema isn't static.

I'm actually not even sure how I feel about dynamic enums, let alone an entirely dynamic schema. :)

@rmosolgo
Copy link
Owner

rmosolgo commented Aug 6, 2018

This is an interesting idea, but I'm going to close the issue because there's no work to do on it in the short term. It will never work with .define-based schemas, and currently, every schema is translated to a .define-based schema when it's executed. So, maybe after we rewrite execution (#1730) more possibilities will be open to us. It's definitely a goal of mine to reduce the various kinds of caching if we can get away with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants