AEB-labs / graphql-weaver

A tool to combine, link and transform GraphQL schemas
MIT License
240 stars 20 forks source link

Some implementations lost after weaveSchemas #59

Closed igorlesnenko closed 5 years ago

igorlesnenko commented 5 years ago

Before weave schema:

 _implementations: 
   { _Document: [ Navigation, Page, Page_template, Test ],
     _Linkable: 
      [ Navigation,
        Page,
        Page_template,
        Test,
        _ExternalLink,
        _FileLink,
        _ImageLink ],

Running:

 weaveSchema = await weaveSchemas({
      endpoints: [
        {
          namespace: 'cms',
          typePrefix: 'CMS',
          schema: anotherSchema,
        },
        {
          schema: localSchema,
        },
      ],
    });

Result:

_implementations: 
   { CMS_Document: [ CMSNavigation, CMSPage, CMSPage_template, CMSTest ],
     CMS_Linkable: [ CMSNavigation, CMSPage, CMSPage_template, CMSTest ],

Below fields are missing:

 _ExternalLink,
        _FileLink,
        _ImageLink

Expected:

_implementations: 
   { CMS_Document: [ CMSNavigation, CMSPage, CMSPage_template, CMSTest ],
     CMS_Linkable: [ CMSNavigation, CMSPage, CMSPage_template, CMSTest, 
CMS_ExternalLink, CMS_FileLink, CMS_ImageLink ],
Yogu commented 5 years ago

We remove types that are neither rechable through a field, nor are implementations of reachable interfaces. This is the implementation. Thus, it would be expected to lose types that not reachable at all. Would this apply for _FileLink and _ImageLink? I could also imagine that it's broken for implementations of interfaces that are in turn only used in implementations of interfaces, or something similar.

Could you post the SDL / type definitions for the relevant types? I'm not quite sure how to read your syntax with _implementations: etc.

igorlesnenko commented 5 years ago

@Yogu I think the issue is here: https://github.com/AEB-labs/graphql-weaver/blob/c7f7a34ef91fc6a99609ef267cdf14e59b859b17/src/pipeline/namespaces.ts#L25

We need to also provide types types: Object.values(schema.getTypeMap())

With this everything works fine

Yogu commented 5 years ago

Oh, so the NamespaceModule does not use transformSchema at all. I guess your fix should be fine - we only wrap the root types (and reuse the old types as the field types), so we don't need to delete unused types here.

Could you submit a pull request with this change? A testcase also would be awesome, maybe you could just copy the interfaces regression test (along with interfaces.graphql and interfaces.result.json) and add a namespace to the endpoint config. If not, a PR with just the fix would also be cool :)