Make adaptor in PgServiceConfiguration the concrete adaptor instance, not import path

[hannesj]

Description

Currently the exact adaptor to be used is passed in as an import specifier, rather than the concrete module instance. This changes it so that the concrete adaptor is passed in the config, making it possible to bundle the library eg for usage on function as a service platforms.

Fixes #1826 as per suggestion.

Technically this is a breaking change as the configuration format changes, but I'm not sure how widely it is currently used today.

Performance impact

Should not affect

Security impact

Should not affect

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.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 1b7395ca78257f19100eb177426f444f3c583dfc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
graphile-build-pg	Patch
postgraphile	Patch
@dataplan/pg	Patch
graphile-utils	Patch
pgl	Patch
graphile	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[hannesj]

it would not be an exposed interface in the way that this is modelled.

It shouldn't need to be? It only needs to implement the PgAdaptor interface, that we any way need to expose if we want the user to be able to supply their own imported module? I was under the impression that supplying the "correct" adaptorSettings as part of the config was the Typescript trickery mentioned, as that is the only part that can be overridden by declaration merging and for which the name of the module was used.

[benjie]

Ah you're right, we never did make WithPgClient generic, so it does not pass through the adaptor currently and thus the PgClient you get doesn't explicitly support additional methods (though you can cast it to do so, I guess). I'm guessing I gave up trying to get it to work due to lack of time :man_shrugging:

I'll revisit this PR next week, thanks again :+1:

[benjie]

Regarding what I was discussing before, specifically I was referring to this pattern:

https://github.com/graphile/crystal/blob/05265a2d5576430256f796f17cebebf030206afe/grafast/dataplan-pg/src/adaptors/pg.ts#L377-L380

Note that we don't just return a generic PgClient, we return a NodePostgresPgClient. My hope was to hook this up via TypeScript such that users could conveniently use the additional methods on their PgClient of choice (in this case the rawPgClient property) in their plans/etc.

https://github.com/graphile/crystal/blob/05265a2d5576430256f796f17cebebf030206afe/grafast/dataplan-pg/src/adaptors/pg.ts#L64-L66

The plan was to do more declaration merging, e.g. something like:

declare global {
  namespace DataplanPg {
    interface PgClientByAdaptor {
      "@dataplan/pg/adaptors/pg": NodePostgresPgClient
      // Add more here via declaration merging
    }
  }
}

then you can use the adaptor name to look up the client type. I'm not sure it would be possible to do the same using a TAdaptorSettings generic?

[benjie]

I've marked this as a draft; please ping me when you're ready for another review. Thanks for your work on this! :raised_hands:

[hannesj]

Sorry for taking some time with this. I now preserved the approach of having a string-keyed object containing all the different adaptors, and using that string to select which on will be used in a config.

[benjie]

Dear Benjie, the main motivation for this is to improve bundling as outlined in #1826. TypeScript is a secondary concern. As such, I think we should follow your suggestion above:

we should move the TPgClient generic to the end, and add a method so you can pass just the client as a generic and the other generics get inferred automatically.

[benjie]

Rebased on the latest main; did a rough comparison of the diff to see that the rebase went well.

[benjie]

Massively overhauled; I've removed the adaptor from the types itself because it became a bit circular; instead now you just define the argument types and the client types and we imply everything else from that. I've also moved all the new generics to the end and given them all defaults, this makes the change minimally breaking. Looks good to me; thanks for your work on this! :raised_hands: