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

Introduce monorepo architecture with scoped packages #3321

Closed
yaacovCR opened this issue Oct 18, 2021 · 15 comments
Closed

Introduce monorepo architecture with scoped packages #3321

yaacovCR opened this issue Oct 18, 2021 · 15 comments

Comments

@yaacovCR
Copy link
Contributor

Consider dividing this package into @graphql/schema, @graphql/execution in a true multipackage monorepo design instead of the loosely divided current "module" setup.

This would allow much easier forking of the executor for customization, without running into the "multiple versions of graphql issue."

A current alternative that points the way is graphql-executor.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Oct 20, 2021

@yaacovCR Not sure I understand what problem is solved by monorepo design.
Can you provide an example release flow that allows for refactoring code across modules?
We can use #3305 as an example change that touches both @graphql/schema and @graphql/execution packages.

With the monorepo design I see two possibilities:

  1. All packages are updated in lock-step, which is very easy to implement.
  2. Packages are independently versioned.

Which one are you proposing?

This would allow much easier forking of the executor for customization, without running into the "multiple versions of graphql issue."

Can you please elaborate on this one?

@IvanGoncharov
Copy link
Member

@yaacovCR Thinking more about this, what we can do is to replicate graphql-executor but as a build step. Meaning during release we release graphql (with everything) and @graphql/execution( same version and peer dependency on current major release of graphql).
So exact same approach as graphql-executor but done by using babel-transformation.
What do you think about this?

@yaacovCR
Copy link
Contributor Author

It is tricky to manage compatible versions within a monorepo, so we could start with #1. Ideally, you would properly indicate which patch versions are required for every patch release of every module.

the use case is as mentioned above, much easier approach to forking executor.

i am not sure why build step with Babel is easier than monorepo structure.

@yaacovCR
Copy link
Contributor Author

You would generally speaking use a changeset like release workflow to manage this

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Oct 20, 2021

i am not sure why build step with Babel is easier than monorepo structure.

@yaacovCR A couple of reasons, from the top of my head:

  1. There is no standardization on module loading in the JS ecosystem so I don't want to restrict non-node use cases (e.g. Deno, native ESM from browsers, etc.). Provide bundled version of graphql for those platforms provide them with the worst DX.
  2. It complicates tooling, we need to use staff like lerna instead of using npm directly
  3. jsutils is an internal package with private API and no index.ts (by design) but we would need to publish it as a package anyway.
  4. Where stuff like version.ts will leave?
  5. Most important: If end-users need to add 8 separate packages to peerDependencies it provides bad DX and multiple confusion that we currently have over "multiple versions of graphql issue." by a factor of 8.
  6. instanceOf check helps us to prevent bugs due to passing objects between incompatible graphql versions. But for modules like language we can't use instanceOf so that means can't prevent scenarios where one version of @graphql/language is used to produce AST but an incompatible one is used to print AST.

I like your approach with graphql-executor and this is something we can easily do in a few build steps, I can even publish it to a separate branch on every commit similar to npm and deno branch. It means graphql-executor can be a list of patches on top of this branch, hopefully, we can keep this list small and temporary.

the use case is as mentioned above, much easier approach to forking executor.

Can you please list what problems it solves?
Because the only one I'm aware of is to have different import paths.
Do you have any other problems in mind solved by monorepo?

@IvanGoncharov
Copy link
Member

@yaacovCR Also in some situations companies/projects (e.g. today learned Apollo use a vendor copy in one of the projects) need to vendor graphql as a package.
At the moment they can just put the whole repo under vendor/graphql and use paths like vendor/graphql/execution/execute.ts that wouldn't work with monorepo since it requires the build step and some mechanism to inject built packages.

@saihaj
Copy link
Member

saihaj commented Oct 20, 2021

We can publish 2 packages one that is a built verison how we do it today so end users do not need to worry about peer-deps. Other package is the one we used to build it which would allow users to swap out custom module versions.

@IvanGoncharov
Copy link
Member

@saihaj Not sure I understand. Can you please describe it in more detail?
Also please note: by end-user I don't mean only people using this lib directly but also people combining different libraries together, e.g. using apollo-server together with graphql-tools since currently it working only if we put graphql into peerDependencies.

@saihaj
Copy link
Member

saihaj commented Oct 21, 2021

so we make a new package graphql-core which will have all the modules as dependencies @graphql/language, @graphql/executor and etc. When we are releasing we bundle this package up into graphql and we publish it as we normally do. Users who want to customize specific packages they can use graphql-core then they have to manage peer deps for all the packages they decide to use and for others they can continue to use graphql normally. With bundling we can also generate packages specific for deno and for web we can even have a slimmed down version like graphql-web-lite

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Oct 21, 2021

@saihaj That wouldn't work if people use one code copy (graphql package) and pass values to another code copy (@graphql/executor depending on @graphql/schema) we will have:

"multiple versions of graphql issue."

@saihaj Even if we workaround instanceof issue somehow it will be bad because people will generate schema with one version of graphql (e.g. using graphql) pass to some lib (e.g. graphql-tools) that use another version of code (through graphql/execution that depended on @graphql/schema).
With API rich library like graphql (hundreds of public functions accepting dozens of different types and classes) is forcing npm to ensure that node_modules has exactly one version of a code through peerDepedencies.

@IvanGoncharov
Copy link
Member

we can even have a slimmed down version like graphql-web-lite

@saihaj Tree-shaking completely eliminates the problem of the package having files that you don't using.
graphql-web-lite is smaller not because it has fewer files but because the author drop functionality (e.g. parser parses only queries) and because the author rewrote code to optimize it just for code size (e.g. use bunch of regexes).
Both technics are not possible with our goal of being both client/server lib and also being reference implementation.
So I want to be clear that it's not something that can be solved by monorepo.

@saihaj
Copy link
Member

saihaj commented Oct 21, 2021

@saihaj Tree-shaking completely eliminates the problem of the package having files that you don't using.
graphql-web-lite is smaller not because it has fewer files but because the author drop functionality (e.g. parser parses only queries)

yes my suggestion based on the fact that we can bundle custom modules but you mentioned “multiple versions of GraphQL issue” so what I suggested won’t work or maybe 🤔 there is something else we can do

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Oct 22, 2021

Also let's not forget that monorepo has other cons I listed here: #3321 (comment)

I actually like the idea of graphql-executor so I think we just need to make it easier to maintain that's why I suggested babel transform and publishing branch.
But maybe it is worth returning to the previous idea of class with unstable API but publishing it as @graphql/execution (with imports from graphql).
@yaacovCR It solves the biggest problem of "unstable class" API we discussed. What do you think?

@yaacovCR
Copy link
Contributor Author

One of the goals of graphql executor is to allow for easily forking the executor, making small changes, and using the result. I think a Babel transform does not as easily allow for that as does a monorepo managed with changesets or lock step versioning. I don’t see a problem with publishing ‘@graphql/js-utils’ to npm, it’s an internal api but it’s not a secret

@yaacovCR
Copy link
Contributor Author

On much later reflection, @IvanGoncharov , I think your raised a number of important blockers for the above work, especially the internal versioning and intercompatibility issue, the "internal" tools.

I think it would be helpful in the long-term still to move in that direction, but it would certainly require work addressing your above points. I might be able to tackle work on some of those blockers, but not in the short or probably even medium-term.

The overall goal is still excellent imo, i.e. making it easy to plug and play aspects of the graphql pipeline. How to get there is not so clear!

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

No branches or pull requests

3 participants