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

update config docs #520

Merged
merged 9 commits into from
Dec 6, 2024
Merged

Conversation

jcgsville
Copy link
Contributor

Description

A bunch of small updates to the Configuration docs. Wording tweaks for clarity, additions to fill in gaps and make things more consistent across the CLI, TS, and markdown docs.

Partially resolves #453. Should probably update the /library/run docs as well. I'll do so when I get to that page.

Performance impact

None

Security impact

None

Checklist

  • My code matches the project's code style and yarn lint:fix passes.

@jcgsville jcgsville marked this pull request as ready for review December 5, 2024 22:28
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.

Definitely good to clarify a lot of this! I'm filing this review so that I can switch to direct editorial, doing this all in GitHub comments is becoming unwieldy.

src/cli.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/interfaces.ts Outdated Show resolved Hide resolved
website/docs/cli/run.md Outdated Show resolved Hide resolved
@benjie

This comment was marked as resolved.

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.

Massive improvement - thanks! 🙌

@benjie benjie merged commit 0d99bce into graphile:main Dec 6, 2024
7 checks passed
@benjie
Copy link
Member

benjie commented Dec 6, 2024

This is now live 🎉 https://worker.graphile.org/docs/config

@jcgsville jcgsville deleted the chandler/config-doc-tweaks branch December 6, 2024 21:32
@jcgsville
Copy link
Contributor Author

I noticed you took out this bit

Adding the import statement tells TypeScript about the `GraphileConfig` global
namespace and the properties that
[Graphile Worker adds](https://github.com/graphile/worker/blob/c70e9db03f6ad292dcdb833714741363bd78937d/src/index.ts#L158)
to the `Preset` interface. No code from the `graphile-worker` or
`graphile-config` libraries will be included in the output JavaScript for
`graphile.config.ts` above. See the TypeScript docs for more info about
[declaration merging](https://www.typescriptlang.org/docs/handbook/declaration-merging.html)
and
[type-only imports](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export).

Do you think that was a distraction / unnecessary? I thought it might help people to understand how the graphile config stuff works. Maybe better included elsewhere?

@benjie
Copy link
Member

benjie commented Dec 6, 2024

It was good; but when I ran the graphile config print command nothing much was output and I realised it was because we didn’t explicitly add the worker preset to the config (because it’s added for us). By adding the preset explicitly the print command worked and there was no need for the stub import type, which meant there was nothing for the note to relate to.

@benjie
Copy link
Member

benjie commented Dec 7, 2024

Definitely consider adding something like it to the graphile config docs though

@jcgsville
Copy link
Contributor Author

Ah, makes sense. Sounds good 👍

@jcgsville
Copy link
Contributor Author

added the stuff about the graphile config global namespace in my latest graphile config docs pr:

graphile/crystal#2282

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

Successfully merging this pull request may close these issues.

Documentation: detail how to pass a preset to run()
2 participants