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 example issues with type extensions #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

landau
Copy link
Owner

@landau landau commented Feb 22, 2022

Hello friends at The Guild -

Overview

The team I am working with has been using graphql-modules project for almost a year (starting with V1). We thank you and other committers for the efforts put in to the project. We have a concern regarding modularization and testing. In particular, when extending interfaces/types outside of a module's scope. We appreciate any insights you may have regarding our concern.

Problem Space

The problem involves testing fields that extend types or interfaces using a reference to interfaces not defined by the module in test. In other words, when extending a type, the module doesn't know what interface a type implements.

To test extended interfaces/types, a tester defines a query to ensure their schema definition and resolvers are working as expected. However, because testkit.testModule does not have a reference to the externally defined interface it is not possible to test the fields using fragmenting.

To ease this understanding, I've modified the example app found in the graphql-modules repo to reflect our problem. See https://github.com/landau/graphql-modules/pull/1/files . I've also added comments in the demo code to further assist understanding. The key things to note are:

  • type User is now interface User and has an implementing type CommonUser.
    • Common* is a pattern we're using for fields on implementing types so clients can reduce the amount of fragmenting in queries, i.e. CommonUser and LoggedInUser both implement User, but fragmenting would only be necessary for LoggedInUser's extra fields.
  • The Social Network module continues to extend the User with a friends field and has an additional scalar foo
    • CommonUser is also extended with these fields
  • I've added tests for the SocialNetwork to exhibit our goal, but only 1/4 tests pass.
  • I've added extra tooling around testModule which allows callers to also replace extensions for modules that extend interfaces. Though this is not the source of the issue, it does exhibit the same problems with referencing interfaces from external modules.

If you'd like to run the tests in the demo app:

$ node -v: v14.16.1
$ npm -v: 6.14.12

  1. npm install at root of this repo
  2. cd examples/basic; npm install
  3. npx jest

Possible Resolutions

We recognize that our problem could be solved in at least a few ways using testkit.testModule, but some resolutions have their own tradeoffs that we'd like to avoid because they do seem to be acting in accordance with modularity.

  • Re-implement the external interfaces using testModule typeDefs config.
    • Tradeoffs
      • If the externally defined interface (the source of truth) changes, then tests should be updated accordingly.
      • There could be a lot of interfaces needed to be redefined
  • Use testModule inheritTypeDefs config.
    • Tradeoffs
      • If you include a module that also references external types, then you have to include those modules too. Depending on the schema layout, testers can find themselves including every module.
    • Note: This works well for low level interfaces like a node interface.
  • Extend test tooling such that, any time a type is defined, tooling should search the schema for an interface for a matching suffix. This seems like the "cleanest" mechanism to support our use cases.
    • i.e, An external module extends type CommonUser and interface User. replaceExtensions: true strips extends from both definitions and appends implements User to the CommonUser type since it's suffix matches the interface.
    • Tradeoff: Must be careful about naming things in modules
  • Bonus idea: Don't allow modules to reference externally defined interfaces.
    • Tradeoff: Loss of API expressiveness

Thank you for taking the time to review our problem.

@landau landau added the question Further information is requested label Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant