Closed FelixZY closed 2 months ago
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.
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 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.
The syntax in my commits is based on conventional commits.
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)?
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! 😅
No worries, just wanted to make sure it was in there somewhere 🙂👍
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:
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.
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.
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?
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.
Description
This PR primarily attempts to fix an issue where it was not possible to define
adaptorSettings
inmakePgService
(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
yarn lint:fix
passes.yarn test
passes.RELEASE_NOTES.md
file (if one exists).