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

hannesj · 2024-03-07T11:23:02Z

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 · 2024-03-07T11:23:05Z

⚠️ No Changeset found

Latest commit: c394ea4

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

benjie

Thanks for your work on this!

From what I can tell from a quick scan over this (and without trying it out) I think we're losing one of the most powerful features of adaptors (and the primary reason that they exist): the ability to extend your adaptor with additional properties/methods specific to its capabilities. For example, some people might like a pg-promise adaptor which allows them to also use the pg-promise instance directly in their mutations, but though this would still be compatible with PgAdaptor, it would not be an exposed interface in the way that this is modelled. The previous string-based method allowed registering your own adaptors with additional properties/methods by name (which was the "TypeScript stuff" I referred to in #1826 (comment) ).

I wonder if we can achieve both approaches in a cunning way, such as:

adaptor: {
  name: '@dataplan/pg/adaptors/pg',
  module: await import('@dataplan/pg/adaptors/pg')
}

WDYT?

postgraphile/website/postgraphile/config.mdx

hannesj · 2024-03-19T09:06:14Z

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 · 2024-03-19T13:51:35Z

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 🤷‍♂️

I'll revisit this PR next week, thanks again 👍

benjie · 2024-03-27T08:07:30Z

grafast/dataplan-pg/src/index.ts

-      TAdaptor extends
-        keyof GraphileConfig.PgDatabaseAdaptorOptions = keyof GraphileConfig.PgDatabaseAdaptorOptions,
-    > {
+    interface PgServiceConfiguration<TAdaptorOptions = PgAdaptorOptions> {


We should not have this; defaulting to using the adaptor options for the 'pg' adaptor doesn't make sense for the majority of other adaptors, and would be unsafe to use across the codebase.

Removing this default causes cascading failures. I can't push to your branch so I've pushed up a commit here 8d57b63 to address the beginnings of these, but I can't figure out how to get it to pass with solid typings without resorting to any. Please feel free to pull my changes into your branch and continue iterating.

benjie · 2024-03-27T08:12:30Z

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

crystal/grafast/dataplan-pg/src/adaptors/pg.ts

Lines 377 to 380 in 05265a2

    
           export function createWithPgClient( 
        
             options: PgAdaptorOptions = Object.create(null), 
        
             variant?: "SUPERUSER" | string | null, 
        
           ): WithPgClient<NodePostgresPgClient> {

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.

crystal/grafast/dataplan-pg/src/adaptors/pg.ts

Lines 64 to 66 in 05265a2

    
           export interface NodePostgresPgClient extends PgClient { 
        
             rawClient: PoolClient; 
        
           }

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 · 2024-03-27T14:54:31Z

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

Fixes graphile#1826

… some cascading issues

WIP

hannesj · 2024-04-24T07:30:10Z

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

Sorry for the delay in reviewing, I've been head down in rewriting some complex Grafast internals!

~~If the aim of this is that you can pass your custom client type via a generic then we should keep that in mind but~~ make sure that we don't a) break existing code and b) make it harder for people who don't want to replace their PgClient. I think 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.

~~However, if the aim is that using withPgClient should automatically infer the PgClient interface then we should explore other options (we'll still need the generics added):~~

~~Option 1: make the client a generic parameter to the executor, and infer from there automatically.~~

Option 2: guess that we're reading from the context.withPgClient property, and thus read the type from Grafast.Context['withPgClient']. I'm not keen on this option because it's inflexible, and assumes you only have one client.

~~Non-breaking addition of the generics should be able to go through anyway, so we don't need to decide Option 1 or 2 now - we can merge without that.~~

benjie · 2024-05-07T16:32:51Z

grafast/dataplan-pg/src/steps/withPgClient.ts

+export type WithPgClientStepCallback<
+  TPgClient extends PgClient,
+  TData,
+  TResult,


Here's roughly the status quo on TypeScript Playground. Note TData and TResult can be inferred from usage.

Adding a new generic at the beginning seems like it's a breaking change for no specific reason; e.g. if you call withPgClient<SuperPgClient>(...) you'll get the error that withPgClient requires 3 generics. So if we're going to add this I think we should do so via the last position:

Suggested change

export type WithPgClientStepCallback<

TPgClient extends PgClient,

TData,

TResult,

export type WithPgClientStepCallback<

TData,

TResult,

TPgClient extends PgClient = PgClient,

Unless there's a good reason not to?

Further I think we should do a ridiculous TypeScript hack for typing and insert a function that does (essentially) nothing:

const $result = withCustomPgClient<SuperPgClient>()( executor, $data, async (pgClient, data) => { // ^? SuperPgClient } );

This allows you to pass the type for the client whilst still having the other two generics inferred.

https://www.typescriptlang.org/play/?#code/C4TwDgpgBACg5gUQB4QMYFdgHsBOUC8UA3lBAFxTA7rQC+A3AFCiRTJqYCGARgDYQBlYBDAAeACoA+AsSgBnYWArioDRgEsAdsJwAzTqmjwAwr3URtxRlCgBHGjhAAKTjjhyKJYUmAUFOLThVAEoKGBwsAFt1OQhRTk0QSSZaRg1tCD0DaAF0SBwTMwtgUh8LABM5WDhTc0siayhyhMMnUKhNdEjuTJS0iCQwXBKWaAB1dWAAC0K64CERY05eXm4DAGtRRvEAEU5gTgAabYAlCDl0XmBjm3FZ4tLhTUrq2uLj6UInVCLtZXvtIcmvtOMo9gdggRpOEojE4uIzhcrslGAMhjgSrp0JpUMB1FhNFAAO6TGY1X7ACQAkoDJ4valA3YgxmIy7ASRORoDDjYHBhRAoDC8m5QAAkzQOZEaNgAPmxBVw+IJFBJwZxJNKoHKnEyDo8KlVOistVBseUILotBBylAAPwdS68E1mi1Wm0UTQQABumWCItQy1WGwoE2m1IWYCWKzWqE2uqOUAR5zZjOpkmO7XYQp4-AjElZyKsNhwEGA6BwhKNTs4VQSID6jFQBIUpR5uBkVagNa7iS7VXgWcwuCYTc0LdFoPlPJzypEon8gU+sgUIgoACJhAo16oR83MTISWHyXMnNyhbggROgTWQDioN8KdfIfhpA0bD85kwbAB6b9QAB6tqNJwRKcJMUAfsUAB09iZM4Xi0hQnAhF+UAlmWFZdikwS7mOJQAGIyLoTC-jYZFkYB-SDMMUBYjieIEsSpLGOgChRNSVLHg8tIGq8FIcu0Wy3GqIpJki1yMByXIKry-KDsKjTiiCUrkXK8kznm8Yaqp97xvqzyGo6zrPK6no2vanZyi6lpmVAHrer6-qBjG6whqS4aKFGQaxqqzKJgW1yJmmGZQlO2ZKppAWvo06HloSh5km82gNqOLZBIQCUsWxkQcbk+Rpm0nI2GeQ44CKV7AXIt6oPekGAl2z7ReRdXAKhP5-pR5EgWBJQtTBDjwZQiFdihmqxZhnCobQGa4S2ADiMhwCRf7kRRQFAA

That said... It feels like it's the executor itself that should contain information on what the PgClient type is (since that's where the plan that gets it from context() resides). We might need TypeScript code generation for that to work reliably though.

benjie · 2024-05-07T16:34:47Z

postgraphile/postgraphile/__tests__/helpers.ts

        adaptorSettings: {
          connectionString,
        },


You still have adaptorSettings here, but removed it in other places?

benjie · 2024-05-07T16:40:06Z

postgraphile/postgraphile/src/cli.ts

-      preset.pgServices?.[0]?.adaptor ?? "@dataplan/pg/adaptors/pg";
+      preset.pgServices?.[0]?.adaptor ??
+      (await import("@dataplan/pg/adaptors/pg"));

-    const importSpecifier = adaptor.match(/^([a-z]:|\.\/|\/)/i)
-      ? pathToFileURL(adaptor).href
-      : adaptor;
-
-    const mod = await import(importSpecifier);
-    const makePgService = (mod.makePgService ?? mod.default?.makePgService) as (
-      options: MakePgServiceOptions,
-    ) => GraphileConfig.PgServiceConfiguration;
+    const makePgService = adaptor.makePgService;
    if (typeof makePgService !== "function") {


Please re-add the ESM shenanigans avoidance code, otherwise my support burden will increase 😅

let tmp; const adaptor = preset.pgServices?.[0]?.adaptor ?? ((tmp = await import("@dataplan/pg/adaptors/pg")) && tmp.makePgService ? tmp : tmp.default);

benjie · 2024-05-07T16:40:48Z

postgraphile/website/postgraphile/config.mdx

- `adaptor: string` - the name of the module to use as the postgres adaptor;
-  e.g. `@dataplan/pg/adaptors/pg` for the `pg` module
+- `adaptor: PgAdaptor` - the module to use as the postgres adaptor; e.g. the
+  result of `import(@dataplan/pg/adaptors/pg)` for the `pg` module


Suggested change

result of `import(@dataplan/pg/adaptors/pg)` for the `pg` module

result of `import("@dataplan/pg/adaptors/pg")` for the `pg` module

benjie · 2024-05-10T13:23:30Z

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 reviewed Mar 11, 2024

View reviewed changes

postgraphile/website/postgraphile/config.mdx Show resolved Hide resolved

benjie reviewed Mar 27, 2024

View reviewed changes

hannesj force-pushed the main branch from eb9931c to 582759e Compare March 27, 2024 11:24

benjie marked this pull request as draft March 27, 2024 14:53

hannesj and others added 2 commits April 23, 2024 17:26

Make adapter in config the concrete adapter instance, not import path

6dfe5ef

Fixes graphile#1826

Remove invalid default for PgServiceConfiguration generic and address…

2cba7b9

… some cascading issues

hannesj force-pushed the main branch from 83758df to eec829b Compare April 23, 2024 15:04

Preserve string-based approach for selecting adaptor

c394ea4

WIP

hannesj force-pushed the main branch from eec829b to c394ea4 Compare April 24, 2024 07:26

hannesj marked this pull request as ready for review April 24, 2024 07:30

benjie requested changes May 7, 2024

View reviewed changes

benjie mentioned this pull request May 10, 2024

Bundling error when used with NextJS #1826

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hannesj commented Mar 7, 2024

changeset-bot bot commented Mar 7, 2024 •

edited

benjie left a comment •

edited

hannesj commented Mar 19, 2024 •

edited

benjie commented Mar 19, 2024

benjie Mar 27, 2024

benjie commented Mar 27, 2024

benjie commented Mar 27, 2024

hannesj commented Apr 24, 2024

benjie left a comment •

edited

benjie May 7, 2024

benjie May 7, 2024

benjie May 7, 2024

benjie May 7, 2024

benjie commented May 10, 2024

	result of `import(@dataplan/pg/adaptors/pg)` for the `pg` module
	result of `import("@dataplan/pg/adaptors/pg")` for the `pg` module

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

Are you sure you want to change the base?

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

Conversation

hannesj commented Mar 7, 2024

Description

Performance impact

Security impact

Checklist

changeset-bot bot commented Mar 7, 2024 • edited

⚠️ No Changeset found

benjie left a comment • edited

Choose a reason for hiding this comment

hannesj commented Mar 19, 2024 • edited

benjie commented Mar 19, 2024

benjie Mar 27, 2024

Choose a reason for hiding this comment

benjie commented Mar 27, 2024

benjie commented Mar 27, 2024

hannesj commented Apr 24, 2024

benjie left a comment • edited

Choose a reason for hiding this comment

benjie May 7, 2024

Choose a reason for hiding this comment

benjie May 7, 2024

Choose a reason for hiding this comment

benjie May 7, 2024

Choose a reason for hiding this comment

benjie May 7, 2024

Choose a reason for hiding this comment

benjie commented May 10, 2024

changeset-bot bot commented Mar 7, 2024 •

edited

benjie left a comment •

edited

hannesj commented Mar 19, 2024 •

edited

benjie left a comment •

edited