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

Improve graphile export docs #2167

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mrjackdavis
Copy link

Description

Raising this PR early for visibility. Open to feedback as I go

Copy link

changeset-bot bot commented Sep 2, 2024

⚠️ No Changeset found

Latest commit: 062042e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@benjie
Copy link
Member

benjie commented Sep 2, 2024

Thanks so much for contributing some documentation! Just a heads up that this is an unusually busy time for me: I just came back from vacation yesterday and on Saturday I'll be flying to America for GraphQLConf where I have two unfinished talks to present, so it may take me a few weeks to get around to reviewing it fully. I might do a cursory review before then if I get a chance, but it is likely to take a while to get merged. Sorry!

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this! It is a good start; but I put in a lot of effort to make sure that the solutions we build are generally useful where they can be - i.e. someone using any other schema generation framework should be able to export their schema as executable using graphile-export without even having to know PostGraphile exists. Thus any PostGraphile-specific documentation should remain within PostGraphile's documentation.

Similarly, a tool like graphile-export's documentation should detail fully how to use the tool no matter what type of consumer you are; however PostGraphile's documentation should cover specifically how to use the tool with PostGraphile in a simplified form - for example, if we wanted to demonstrate a PostGraphile plugin to automatically export the schema once it's built (like this), that would be in the PostGraphile documentation, not the graphile-export docs.

I hope that makes sense!

(Sorry again that it's taken a while to get around to looking at this.)


# Troubleshooting

## undefined variable `EXPORTABLE`

Our ESLint plugin isn't smart enough to actually `import` the `EXPORTABLE`
helper, so after running the autofix you might end up with "undefined variable
Copy link
Member

@benjie benjie Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the exportable page so as to not break any hyperlinks; this can also act as a reference. E.g. update the end of this paragraph:

You can either `import { EXPORTABLE } from "graphile-export"`, or you can [implement `EXPORTABLE` in your own code](./exportable).

---

# Using EXPORTABLE
As explained in [how it works](./how-it-works.md), plugins must be made compatible to work with graphile export. *This applies to external (npm) plugins and internal (your own) plugins*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the graphile-export package which is independent of graphile-build/postgraphile/etc, so it has no concept of plugins and shouldn't talk about them.

Maybe something like:

As explained in how it works, GraphQL schemas must be made compatible to work with Graphile Export - this includes everything in the schema, including traditional resolvers, type resolvers, plan resolvers, enum values, default values, and everything else that makes up the schema.

Note: if you're using graphile-export to export a PostGraphile schema, you should consult the PostGraphile documentation for specifics - especially regarding how to ensure plugins only make exportable changes to a schema.

[ 7 ]
```

Thus everything that can have these kinds of hidden properties must be wrapped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Thus everything that can have these kinds of hidden properties must be wrapped
Thus everything that can reference values from a parent scope must be wrapped

Comment on lines +10 to +11
The key reason to export your schema is to move schema introspection of postgres
from runtime to build time. This results in:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

graphile-export knows nothing of Postgres, and is not specific to PostGraphile.

Schema exporting was designed for systems that generate a GraphQL schema dynamically.
This dynamic generation can often consume a lot of compute, and may not be a cost that
you want to incur on every startup - instead, you can export your dynamically
generated schema as executable code at build time; then at run time you just need to
run the final code without any of the underlying computations that shaped it. This
has many benefits:

things such as classes). In JavaScript you can see the source code of a function
by calling `.toString()` on it:
- Faster startup time
- Reduced thundering herd on the database in the event of an outage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Reduced thundering herd on the database in the event of an outage
- Reduced thundering herd in the event of mass server restarts

by calling `.toString()` on it:
- Faster startup time
- Reduced thundering herd on the database in the event of an outage
- Much faster cold starts for Lambda
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Much faster cold starts for Lambda
- Much faster cold starts for serverless environments such as AWS Lambda

Comment on lines +18 to +20
Previously, in Postgraphile 4, export took the form of encoding the Postgres
schema to JSON. Postgraphile 5 takes this further and generates runtime code,
further cutting startup times.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be part of the graphile-export documentation; add it to the PostGraphile pages if you feel it warrants detail (though I'd personally keep it to the V4 migration guide).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with paragraph below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🌱 In Progress
Development

Successfully merging this pull request may close these issues.

2 participants