#2096: allow `postgraphile/adaptors/pg` settings to be set in `makePgService`

[FelixZY]

Description

This PR primarily attempts to fix an issue where it was not possible to define adaptorSettings in makePgService (see discussion on discord).

The PR also aims to improve the related documentation at https://postgraphile.org/postgraphile/next/config#adaptorsettings however, this is not yet completed.

Related issue is #2096

Please note that I'm having problems getting the tests to run locally so I cannot check that they pass - I'll have to look at the CI builds.

Performance impact

unknown, likely none.

Security impact

unknown, likely none.

Checklist

• [x] 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).
• [x] If this is a breaking change I've explained why.

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 8b2cfe56382df0da23b338b2a12a81340495ea81

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

[FelixZY]

@benjie I'm planning on making documentation fixes (and probably adding a changeset?) but I'd like to confirm that these changes are in line with your expectations and desires first.

[FelixZY]

The syntax in my commits is based on conventional commits.

[FelixZY]

Hey @benjie, have you had time to take s look at this and decide whether it's in line with your plans (i.e. should I finish it)?

[benjie]

Hi @FelixZY, so sorry this is taking me a while to get to, GraphQLConf CFP process took over my life for a bit and I really had to catch up on client work, then I spent a while going through my GitHub notifications in reverse chronological order, and in doing so discovered a bug in error handling in Grafast. Reviewing this PR was next on the list after that, I wasn’t expecting it to take me so long but I’ve had to do a whole host of refactoring to support making the fix in a clean way… I’ll try and review this PR tomorrow before continuing work on the error stuff. Sorry again — I’m not ignoring you, just a little overwhelmed with things to do! 😅

[FelixZY]

No worries, just wanted to make sure it was in there somewhere 🙂👍

[benjie]

Thanks for looking into this! I had to really think about why I'd built things this way, and in the end I took an alternative approach that better reflects the intent of the two functions taking different arguments. I've outlined the reasoning in https://github.com/graphile/crystal/pull/2121

I think your main aim was that you should be able to set poolConfig via makePgService(), so this is the main thing that alternative PR unlocks (hopefully... I've not tested it).

Please note that although I'm not merging this PR, the work you did was useful helping me to introspect on how and why the system works the way it does, and your investment of time also showed that this was a feature worth prioritising. Thanks :raised_hands:

[FelixZY]

I'm happy that the feature (passing poolConfig via makePgService) will be available and appreciate you taking the time to look into this. For me, the end result is most important and I'm not too sad about you creating another PR instead of mine. Indeed, you have better insight into the system and I did stop early because I wanted to be sure about the implementation before investing too much time. I'm grateful for you taking the time to read over my code, reflect and merge - what I expect to be - an even better solution @benjie 🙂

One final note: I did read through your PR. While things looked good, I still would like to raise the point made in the commit message of https://github.com/graphile/crystal/pull/2104/commits/91a505e3d78d1ec1e3b24bdea0e57d98df587b9d, that it might make sense to go from PgAdaptors -> PgAdaptor (see the commit message for a full motivation). If you disagree, that's fine, but I want to make sure it has been considered and consciously rejected in that case.

[benjie]

We should probably rename it to something that better represents what it is (GraphileConfig.PgAdaptorDetailsLookup?), but I want people to be able to refer to the adaptors by the name (with auto-complete) rather than having to know what things to import to use them.

[FelixZY]

Makes sense. Something I don't like right now though is that there is nothing restricting the values in PgAdaptors from being just anything. Also, there's not an easy to declare your own adaptor and get type hints for what to implement (as you would get with PgAdaptor). Maybe a hybrid solution would cover both use cases?

[benjie]

I'm confident that's an issue that we can solve, for example by exporting the interfaces the adaptor must conform to and having them implement satisfies when defining the type. I suspect the number of people implementing custom adaptors will be significantly (orders of magnitude) smaller than the people consuming it, so I'd rather ensure that it's easy for consumers even if it means adaptor authors have to add an extra couple of lines to their code and read a little bit of documentation.