-
-
Notifications
You must be signed in to change notification settings - Fork 569
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
Documentation for makePgService({adaptorSettings})
appear to be outdated
#2096
Labels
Comments
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
…* of pass-through options
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
These options are currently exposed (see [grafast/dataplan-pg/src/interfaces.ts:420]( https://github.com/graphile/crystal/blob/ab3ac2259d00af95be81fdd8916bff5e9340be01/grafast/dataplan-pg/src/interfaces.ts#L420) ) but not described by the documentation.
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
…daptor` The previous solution for defining adaptors had the following documentation-only requirement: ```ts // grafast/dataplan-pg/src/index.ts declare global { namespace GraphileConfig { // ... interface PgAdaptors { /* * Add your adaptor configurations via declaration merging; they should * conform to this: * * ``` * [moduleName: string]: { * adaptorSettings: { ... } | undefined; * makePgServiceOptions: MakePgServiceOptions & { ... }; * client: PgClient & MyPgClientStuff; * }; * ``` */ } ``` This means there was no way to actually enforce that an entry in `PgAdaptors` conformed to the required type. Also, one way to read the `PgAdaptors` interface (which is how I read it at first) is that `PgAdaptors[keyof PgAdaptors]` is a `PgAdaptor`. However, this turned out not to be true as `PgAdaptor` was defined separately and unrelatedly in `grafast/dataplan-pg/src/pgServices.ts`. In my opinion, it would make more sense to define a strict `PgAdaptor` interface rather than a loose and unenforceable `PgAdaptors` interface. This commit therefore restructures `PgAdaptor` into a proper and stricter adaptor type and drops the `PgAdaptors` interface completely. Some related discussions on accessing the generic arguments of an interface: * https://discord.com/channels/508357248330760243/1251973719933325373/1251973719933325373 * https://stackoverflow.com/questions/78629899/ts2344-type-inferred-from-property-on-generic-argument-does-not-satisfy-the-co BREAKING CHANGE: `PgAdaptors` has been replaced with `PgAdaptor`
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
… set in `makePgService` graphilegh-2096 refers to an issue where I was unable to access e.g. `adapterSettings` in `makePgService`. After some short [discussion]( https://discord.com/channels/489127045289476126/498852330754801666/1251548107859034193 ) on discord, @benjie [suggested]( https://discord.com/channels/489127045289476126/498852330754801666/1251556573508010054 ) that this is likely a bug in `makePgService` that should be addressed. The [documentation]( https://postgraphile.org/postgraphile/next/config#adaptorsettings ) states that: > Every adaptor should expose a `makePgService` helper function [...] This "base" `makePgService` function is described to have some shared options, such as `connectionString`. However, > Each adaptor may additionally accept any other options it likes [...] I therefore interpret that the `makePgService` should accept `adapterSettings` directly and not via an `adapterSettings` subkey. This commit therefore expands `PgAdaptorMakePgServiceOptions` to include all values from `PgAdaptorSettings`, allowing these properties to be set in `makePgService` exposed by the `postgraphile/adaptors/pg` adaptor.
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
…daptor` The previous solution for defining adaptors had the following documentation-only requirement: ```ts // grafast/dataplan-pg/src/index.ts declare global { namespace GraphileConfig { // ... interface PgAdaptors { /* * Add your adaptor configurations via declaration merging; they should * conform to this: * * ``` * [moduleName: string]: { * adaptorSettings: { ... } | undefined; * makePgServiceOptions: MakePgServiceOptions & { ... }; * client: PgClient & MyPgClientStuff; * }; * ``` */ } ``` This means there was no way to actually enforce that an entry in `PgAdaptors` conformed to the required type. Also, one way to read the `PgAdaptors` interface (which is how I read it at first) is that `PgAdaptors[keyof PgAdaptors]` is a `PgAdaptor`. However, this turned out not to be true as `PgAdaptor` was defined separately and unrelatedly in `grafast/dataplan-pg/src/pgServices.ts`. In my opinion, it would make more sense to define a strict `PgAdaptor` interface rather than a loose and unenforceable `PgAdaptors` interface. This commit therefore restructures `PgAdaptor` into a proper and stricter adaptor type and drops the `PgAdaptors` interface completely. Some related discussions on accessing the generic arguments of an interface: * https://discord.com/channels/508357248330760243/1251973719933325373/1251973719933325373 * https://stackoverflow.com/questions/78629899/ts2344-type-inferred-from-property-on-generic-argument-does-not-satisfy-the-co BREAKING CHANGE: `PgAdaptors` has been replaced with `PgAdaptor`
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
… set in `makePgService` graphilegh-2096 refers to an issue where I was unable to access e.g. `adapterSettings` in `makePgService`. After some short [discussion]( https://discord.com/channels/489127045289476126/498852330754801666/1251548107859034193 ) on discord, @benjie [suggested]( https://discord.com/channels/489127045289476126/498852330754801666/1251556573508010054 ) that this is likely a bug in `makePgService` that should be addressed. The [documentation]( https://postgraphile.org/postgraphile/next/config#adaptorsettings ) states that: > Every adaptor should expose a `makePgService` helper function [...] This "base" `makePgService` function is described to have some shared options, such as `connectionString`. However, > Each adaptor may additionally accept any other options it likes [...] I therefore interpret that the `makePgService` should accept `adapterSettings` directly and not via an `adapterSettings` subkey. This commit therefore expands `PgAdaptorMakePgServiceOptions` to include all values from `PgAdaptorSettings`, allowing these properties to be set in `makePgService` exposed by the `postgraphile/adaptors/pg` adaptor.
5 tasks
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
…of `PgAdaptor`
FelixZY
added a commit
to FelixZY/graphile-crystal
that referenced
this issue
Jun 23, 2024
…of `PgAdaptor`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
The documentation at https://postgraphile.org/postgraphile/next/config#adaptorsettings is outdated. Specifically,
adaptorSettings
,poolConfig
andsuperuserConnectionString
(those are the once I've tried, can't speak for the rest) are no longer available in5.0.0-beta.26
.Additional context
See this discussion in discord: https://discord.com/channels/489127045289476126/498852330754801666/1251548107859034193
The text was updated successfully, but these errors were encountered: