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

Added v1 of the Customization Summary page, for https://github.com/gr… #2004

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

Conversation

machineghost
Copy link

NOTE: This commit has identical v4/v5 versions of the page. Also it doesn't address mutation customization.

Description

Documentation improvement: graphile/graphile.github.io#407

Performance impact

n/a

Security impact

n/a

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

…hub.io#407.  NOTE: This commit has identical v4/v5 versions of the page.  Also it doesn't address mutation customization.
Copy link

changeset-bot bot commented Mar 24, 2024

⚠️ No Changeset found

Latest commit: 199e1c6

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

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.

This is excellent; I've gone through and done some editorial for accuracy, clarity, flow, and factoring in feedback that I've received over the years. I've not looked at the V4 version since you say it's a copy; but the V5 and V4 versions will ultimately differ since V5 does things in fundamentally different ways. Also note that V5's docs are still broadly in V4's format, but they will be restructured before release (e.g. the pages on schema/server plugins will be merged/restructured to fit the new system) so links to those will be updated over time.

This doesn't need much work to be mergeable, maybe just a few more pointers to other things in the sidebar?

@machineghost
Copy link
Author

I've gone through and done some editorial for accuracy, clarity, flow, and factoring in feedback that I've received over the years.

Looks great!

You're not wrong in what you say, but what I was referring to was that some people get put off by the term "plugins" before they even look at the documentation. For example, telling someone they can "extend the schema through plugins" is different to telling someone that they can "extend the schema with their own types and resolvers" even though under the hood the mechanics of the latter uses the former.

Ah, I completely agree: in general in libraries, straightforward features handle a library's basic customization, while plug-ins handle exceptional/more complex customization (with added complexity/difficulty in implementation).

By saying "use plug-ins to do basic stuff in PostGraphile" I could see how that sounds like "PostGraphile doesn't have a features for basic customization, so you have to do something harder".

unification of the plugins system (no more "schema plugins" vs "server plugins" - just "plugins" now)

I was pleasantly surprised to notice this in the docs, but I wasn't sure if the library itself had changed, or just how you described things. Definitely an improvement!

For example I'm planning to change makeExtendSchemaPlugin to just be called extendSchema, etc - the fact it's a plugin factory is irrelevant to most users, they just want to add stuff to their schema, and plugins: [extendSchema({typeDefs, plans})] likely makes more intuitive sense than plugins: [makeExtendSchemaPlugin({typeDefs, plans})] even though it's literally just a different name.

✺◟(^∇^)◞✺

Conceptually there's lots of other "customization system" terms out there (eg. "middleware" in Express) ... but the moment you have such a system at all, you've already going one abstraction layer away from the idea. In this case, the core idea is "I want to tweak the schema", so why have a plug-in that tweaks the schema when you can just have a schema-tweaking option/feature in the app itself?

Great idea.

I've not looked at the V4 version since you say it's a copy; but the V5 and V4 versions will ultimately differ since V5 does things in fundamentally different ways.

Question: when is v5 launching? Because if it's anytime soon, maybe there's no need to even add this page to the v4 docs (it's more directed at new learners, who will likely be using the newest main version)?

Also, I'm unclear on what the relevant changes were from v4 to v5 (I thought there were just some link changes in the docs). If you do want a v4 page, I could use your assistance enumerating the differences (that impact the page) between versions.

maybe just a few more pointers to other things in the sidebar

Sorry, could you elaborate on this a bit more: what things did you have in mind?

@benjie
Copy link
Member

benjie commented Apr 2, 2024

Question: when is v5 launching?

Here's some of the tasks that need completing before it can go live: https://github.com/orgs/graphile/projects/3/views/4

Also, I'm unclear on what the relevant changes were from v4 to v5

A general overview is available here: https://postgraphile.org/postgraphile/next/migrating-from-v4/v5-new-feature-summary

We also have a migration guide for moving from V4 to V5: https://postgraphile.org/postgraphile/next/migrating-from-v4/

Generally you can look at the changes I made in the commits above, and then just undo any that you know to be untrue for V4.

maybe there's no need to even add this page to the v4 docs (it's more directed at new learners, who will likely be using the newest main version)?

I'd be perfectly happy adding this to V5 only; or adding it to V5 and back-porting it to V4 as a separate effort.

maybe just a few more pointers to other things in the sidebar

Sorry, could you elaborate on this a bit more: what things did you have in mind?

Oh, just anything else you feel like talking about. There's commented content, for example. Also one thing this doesn't address currently is the use-case-based direction, for example: "I want to add a mutation, how do I do that?" The answer is: use a custom mutation (database) function, or use makeExtendSchemaPlugin; but currently though we have those answers we don't group them together under the question. I'm thinking that maybe an FAQ-style "common tasks" section at the bottom that points to the explanations above would be wise. "How to add a mutation" / "How to add a query field" / "How to add a subfield" / "How to add documentation" / "How to remove a field" / "How to achieve 'soft delete'" / "How to get rid of 'nodes'" / ...

@machineghost
Copy link
Author

I noticed this in the code:

"Computed column" functions should have been called "custom field" functions

Why not just change the term for 5.0 then? You add one prominent line to the release notes saying "Computed columns are now custom fields", and a note at the top of the custom fields page saying that they used to be called computed columns, and then all that's left is one big find/replace.

I'd be perfectly happy adding this to V5 only; or adding it to V5 and back-porting it to V4 as a separate effort.

Let's plan to do the second one for now, and in the worst case scenario it winds up being the first.

Also one thing this doesn't address currently is the use-case-based direction, for example: "I want to add a mutation, how do I do that?" The answer is: use a custom mutation (database) function, or use makeExtendSchemaPlugin; but currently though we have those answers we don't group them together under the question. I'm thinking that maybe an FAQ-style "common tasks" section at the bottom that points to the explanations above would be wise. "How to add a mutation" / "How to add a query field" / "How to add a subfield" / "How to add documentation" / "How to remove a field" / "How to achieve 'soft delete'" / "How to get rid of 'nodes'" / ...

Agreed. At first I thought "well it's all so simple now, you don't really need that use-case-based stuff", but after adding it back I really think it improves things. Let me know what you think of the latest push, 199e1c6

@benjie
Copy link
Member

benjie commented Apr 11, 2024

(Just a note to say I appreciate your work on this but I’m focused on the early exit feature currently and it’s taking all my brain space so I won’t be able to review this for a few days.)

@benjie
Copy link
Member

benjie commented Apr 22, 2024

Thanks again for your work on this, and your patience waiting for me to get back to you. I think this work will result in significant improvements for users, particularly people new to PostGraphile! 🙌

Why not just change the term for 5.0 then? You add one prominent line to the release notes saying "Computed columns are now custom fields", and a note at the top of the custom fields page saying that they used to be called computed columns, and then all that's left is one big find/replace.

The reason, as with most minor things like this, is because there's too much other stuff to do and it's relatively low priority. The concept of "computed column function" is strung throughout the entire system: not just the documentation but also the internal APIs and comments in the source code, the test fixtures and test snapshots, and so on. Not just that but also the help messages that I can auto-reply with on Discord and usage in other projects such as Graphile Starter and so on.

Also, I'm not sure that "custom field" functions is the right name either; e.g. "custom query" and "custom mutation" functions are also both defining "custom fields". The thing about a "computed column" function is that it adds to a type that represents a table, as opposed to adding to the root Query or Mutation types.

There's generally no "just" doing anything when it comes to renaming things. If you wanted to take on this change, please do! But please be sure that you do it well, including figuring out what the correct name should be.

Let's plan to do the second one for now, and in the worst case scenario it winds up being the first.

Agreed 👍

Let me know what you think of the latest push, 199e1c6

I think the "best for ..." parts should be removed; they're highly debatable. I like the idea behind them (i.e. giving users guidance when to choose which) but for example I would very rarely recommend a view over a function, even when no additional SQL logic is needed. (See below for more on this topic.)

"simply"/"just"

Avoid these terms, if a reader doesn't know how to do these things then they can come off as dismissive and lead to frustration if they don't find the task to be as simple as the prose suggests. Generally this term can just be deleted.

Adding New Query Sub-Fields

Change this to "Adding a field to a type" or similar.

"Root-Level"

When we talk about root level fields, we're talking about fields that you can query directly on the Root Operation Types. I think some of your text is referring to fields on types (e.g. a User type representing the users table) - these are not "root level". Thus "Table-based Root-Level Fields" should probably be something like "Adding a field to a table-derived type", though care must be taken because this could refer to both the object type and also the input type for create (UserInput) and update (UserPatch) and similar (UserBase, UserFilter, etc) types.

"Best for"

Expanding on my previous point, but the value that the "best for" offers, I think we should maybe add a section ## General guidance at the bottom, that gives general guidance (what follows is a very rough first draft):


General guidance

If you need to store data, then your first choice will generally be a table. Adding a table, assuming it has the correct permissions, will automatically add fields in the relevant places, and you can gain more fields in the ways detailed above (foreign key constraints, unique/primary key constraints, additional columns, functions, plugins, etc).

If you are deriving data from data that you already have stored then you have more choices: view, database function, or schema extension. In general, you should pick whichever you prefer, but be aware of the following:

  • Views cannot accept parameters and require annotations to make them behave more table-like; only use these where table-like behavior is desired (for example when building a versionless facade to some underlying mutable database resource)
  • Functions can accept parameters but have significant performance overhead if implemented poorly, so ensure you're familiar with inlining of PostgreSQL functions and prefer writing your functions in LANGUAGE sql. Procedural programming constructs such as IF and LOOP should generally be avoided in favour of declarative SQL constructs. Functions can also not be INSERT-ed into, UPDATE-d or DELETE-d, though you can expose additional functions that perform this functionality.
  • Schema extensions are more powerful than views and functions and give you stronger control over performance (e.g. forcing inlining, or even moving calculations to JS rather than computing in the database), but to master them you need familiarity with JS, SQL, and the Grafast planning system. Also, since they are not in the database, they can only be used by consumers of GraphQL.

For simple scalars (e.g. deriving fullName by concatenating first_name and last_name) it commonly makes sense to use a database function. Otherwise, we recommend that you start with whatever you are most comfortable with. If you start to face performance issues or you discover the need for functionality that's not compatible with your chosen pattern then you should switch to writing a plugin, and you should be able to do this in a way that does not break your schema. Note that both view and function support are achieved via plugins, so anything they can do can also be achieved via a plugin.

@jwalkerinterpres
Copy link

On vacation this week, but will incorporate the above when I get back. 👍

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.

None yet

3 participants