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

Add graphql-java-extended-scalars dependency management #242

Closed
marceloverdijk opened this issue Jan 2, 2022 · 7 comments
Closed

Add graphql-java-extended-scalars dependency management #242

marceloverdijk opened this issue Jan 2, 2022 · 7 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@marceloverdijk
Copy link
Contributor

It would be nice to add graphql-java-extended-scalars to a project without the need to mention the version:

implementation 'com.graphql-java:graphql-java-extended-scalars'

Rationale is that the extended scalars project is tight to the GraphQL version being used (or at least it is up to now) so it can get out of sync.

E.g. adding:

implementation 'com.graphql-java:graphql-java-extended-scalars:17.0'

And spring-graphql might one day upgrade to GraphQL version 18.

@jord1e
Copy link
Contributor

jord1e commented Jan 2, 2022

I am against this to be honest

This library should really support the standardand not anything else . (And maybe the Apollo Federation specification as a separate module, it is ubiquitous and has large community support https://github.com/apollographql/federation-jvm / https://github.com/apollographql/apollo-federation-subgraph-compatibility)

Support for more scalars has stalled a long time ago (and for good reason!):
graphql/graphql-spec#579
https://github.com/apollographql/apollo-federation-subgraph-compatibility

Also
https://www.graphql-java.com/blog/introducing-extended-scalars

This is not defined by the graphql specification per se so we are reluctant to add it to the core library and then have it turn up later as an officially specified type.
But it really is a badly needed type in your GraphQL arsenal and hence graphql-java-extended-scalars was born https://github.com/graphql-java/graphql-java-extended-scalars
This will be a place where we can add non standard but useful extensions to GraphQL Java.

The library could be deprecated at any time.
Also, scalars need to be implemented on every client side app:

  • the aforementioned DateTime scalar as well as a Date and Time scalar
  • A Object scalar or sometimes know as a JSON scalar that allows a map of values to be returned as a scalar value
  • Some numeric scalars that constrain the values allowed such as PositiveInt
  • A Regex scalar that allows a string to fit a regular expression
  • A Url scalar that produces java.net.URL objects at runtime

If you REALLY need these specific scalars, you can just include the library yourself in my honest opinion.

What I am especially scared of is situations like these in Spring, if Spring GraphQL does become the "standard":
Netflix/dgs-framework#659
graphql-java/graphql-java-extended-validation#52
graphql-java/graphql-java-extended-validation#53

@bclozel bclozel added the status: waiting-for-triage An issue we've not yet triaged label Jan 3, 2022
@marceloverdijk
Copy link
Contributor Author

Honestly, for me the graphql-java-extended-scalars Date scalars are very useful.

An RFC-3339 compliant date scalar that accepts string values like 1996-12-19 and produces java.time.LocalDate objects at runtime.

I'm not saying graphql-java-extended-scalars should be added automatically, only that when people want to use it they should not need to specify the version so Spring GraphQL takes the right (compatible) graphql-java and graphql-java-extended-scalars versions.

What do you think @andimarek ?

@bclozel
Copy link
Member

bclozel commented Jan 3, 2022

There's a policy at the Spring Boot level that states that we won't manage a dependency unless we have a specific support for it. The rationale is: if we don't test for incompatibilities with our own code, we can't manage a dependency as we wouldn't be providing any guarantee to the developers. Given how the registration is done for those scalars, I can't think of anything a this time to support in Spring Boot.

Those scalars seem indeed useful in their own right - are there really compatibility issues between both artifacts? If so, why aren't graphql-java and graphql-java-extended-scalar managed in a dedicated BOM published by the graphql-java org? In this case, Spring Boot could import that BOM directly.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jan 3, 2022
@rstoyanchev
Copy link
Contributor

com.graphql-java:java-dataloader is another one that's closely associated with and used with graphql-java that could potentially be a part of a bom.

@jord1e
Copy link
Contributor

jord1e commented Jan 5, 2022

com.graphql-java:java-dataloader is another one that's closely associated with and used with graphql-java that could potentially be a part of a bom.

DataLoader is already implicitly included via a GraphQL Java api dependency:

https://github.com/graphql-java/graphql-java/blob/1ee9630c7d25ccb55a5b1d9b1ccf7bf7895a8c7f/build.gradle#L92

https://search.maven.org/artifact/com.graphql-java/graphql-java/17.3/jar

    <dependency>
      <groupId>com.graphql-java</groupId>
      <artifactId>java-dataloader</artifactId>
      <version>3.1.0</version>
      <scope>compile</scope>
    </dependency>

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 5, 2022
@bclozel
Copy link
Member

bclozel commented Jan 5, 2022

@jord1e It's not a matter of transitive dependency; if a project publishes several artifacts and has compatibility concerns, it can publish a BOM that developers can use for dependency management.

A consumer project could depend on graphql-java and have another dependency bringing transitively java-dataloader with a different version.

Spring Framework (and many other projects) publish BOMs to help developers in that regard, even if artifacts depend on each other. See https://search.maven.org/artifact/org.springframework/spring-framework-bom/5.3.14/pom

@bclozel
Copy link
Member

bclozel commented May 13, 2022

I'm going to close this issue as the auto-configuration support has since moved to Spring Boot. Also, we don't intend to manage this dependency right now.

@bclozel bclozel closed this as completed May 13, 2022
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

5 participants