Skip to content
This repository has been archived by the owner on Dec 19, 2019. It is now read-only.

Move GraphQL schema files to graphql area #922

Closed
buskamuza opened this issue Sep 10, 2019 · 8 comments
Closed

Move GraphQL schema files to graphql area #922

buskamuza opened this issue Sep 10, 2019 · 8 comments
Labels
Architects: approved Issue is checked and approved by architecture team. framework

Comments

@buskamuza
Copy link
Contributor

buskamuza commented Sep 10, 2019

GraphQL schema files make sense only in graphql area of Magento application.

AC:

  • all schema.graphqls files are moved under graphql area folder
  • validate that application behaves as expected in case a module (for example, an extension) that extends core GraphQL schema is still located in global area
  • Ensure GraphQl style sniffs work properly after the change MC-19366: Adds GraphQL sniffs magento-coding-standard#141

This change should not introduce any breaking changes as files from global and graphql areas should be collected and merged during a GraphQL request. Extensions and customizations that use old convention and have GraphQL schema files located in global area should continue working as before.

@antonkril
Copy link
Contributor

Don't think we should do it now:

  • BC violation
  • in an instance that only serves Graphql and only has Graphql modules installed, there is no benefit of Graphql area

@Vinai
Copy link
Contributor

Vinai commented Sep 22, 2019

This would be a big BC break providing very little benefit from my point of view. I agree with @antonkril that they should remain where they are.

@okorshenko
Copy link
Contributor

Closing for now. Once we will have agreement from all parties, we can reopen this ticket.

@buskamuza
Copy link
Contributor Author

in an instance that only serves Graphql and only has Graphql modules installed, there is no benefit of Graphql area

@antonkril , we already have graphql area.

@buskamuza
Copy link
Contributor Author

This would be a big BC break providing very little benefit from my point of view.

I agree, the benefit is insignificant and if it's going to break somebody it's not worth it.
Which scenario will be broken if we move the files?

@Vinai
Copy link
Contributor

Vinai commented Oct 19, 2019

Any existing module that has schema.graphqls files in etc/ would no longer work.
Modules that aim to support multiple Magento versions would have to duplicate the files in etc/ and etc/graphql which would be an increase in maintenance effort and could likely lead to bugs.

@buskamuza
Copy link
Contributor Author

@Vinai , why? I don't propose to modify behavior of the application. Both etc and etc/graphql should be supported as schema location.
The only case when I see something can be broken is if somebody tries to load schema in non-graphql area, which I don't see as a valid use case. I can update the description if this clarification makes more sense. Or do you think somebody will be broken anyways?

@buskamuza
Copy link
Contributor Author

I described it as:

Extensions and customizations that use old convention and have GraphQL schema files located in global area should continue working as before.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Architects: approved Issue is checked and approved by architecture team. framework
Projects
None yet
Development

No branches or pull requests

6 participants