Add context to relay resolvers #4704

Markionium · 2024-06-04T12:07:46Z

This is a draft PR to implement an approach mentioned in #4000.

I've currently put the resolverContext on the Store instead of the Environment. It seems that the Store might be a better place than the Environment?

If defined on the environment the context would need to go from the environment into the store down to the reader? Since a store could be used by different environments if the resolverContext would be on the environment that might cause more issues than it being on the store?

The following is an example of how this would be used.

One would provide the context as follows.

const store = new LiveResolverStore(source, {
    resolverContext: {
      counter: createObservableCounter(),
      world: "World!"
    },
  });

Then the resolvers could use the context similar to readFragment

/**
 * @RelayResolver Query.hello_context: String
 *
 * Say `Hello, ${world}!`
 */
import {resolverContext} from '../../ResolverFragments';

function hello_context(): string {
  const world = resolverContext<{world: string}>().world;
  return `Hello, ${world}!`;
}

/**
 * @RelayResolver Query.hello_world_with_context: String
 * @live
 *
 * Say `Hello ${world}!`
 */
function hello_world_with_context(): LiveState<string> {
  const dependency = resolverContext<{greeting: string}>();

  return {
    read() {
      return `Hello ${dependency.greeting}!`;
    },

    subscribe(callback) {
      return () => {
        // no-op
      };
    },
  };
}

facebook-github-bot · 2024-06-04T12:07:51Z

Hi @Markionium!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

captbaritone · 2024-06-04T21:55:54Z

Hey @Markionium thanks for picking this up!

I agree that attaching the context to the store is the right choice.

However, rather than accessing the context via a magic function, my preference would be that the context object get passed in as the third argument to the resolver function. This would keep alignment with a traditional graphql-js style resolver function.

/**
 * @RelayResolver User.bestFriend: User
 */
export function myResolver(user, _, ctx) {
    return ctx.userById(user.bestFriendId);
}

In terms of type safety, I think we can allow users to provide a module/export name in the compiler config for the project. We can then include that type in our generated type assertions (note that these are not yet built for TypeScript, but it should be pretty easy to add if you are interested in another project).

You still need to ensure your compiler config and the store you create actually match, but that's something you can get right once and in one place so probably an okay end state.

Beyond that we are also exploring a more light-weight way to define resolvers using an approach similar to Grats. In that world it could be even simpler, and work similar to how it works in Grats (see the docs here).

captbaritone · 2024-06-12T23:56:31Z

Connecting the dots. @alloy and I have been discussing this a bit more on this Gist: https://gist.github.com/alloy/f0c9c90ff7a28f3b17850021488979d5

captbaritone · 2024-08-01T18:45:08Z

Looks like this is getting close, but still in draft. Give me a ping when this is ready for review! Excited to see this!

drewatk · 2024-08-08T16:57:25Z

Hey @captbaritone! We're ready for your review here. @Markionium can you please un-mark as draft?

Markionium · 2024-08-09T05:44:12Z

packages/react-relay/__tests__/__generated__/LiveResolversTest14Query.graphql.js

+  _: void,
+  _: void,
+  context: LiveResolverContextType,
+) => LiveState<?number>);


This _ for both might cause duplicate identifier issues (at least in typescript it would)

We could probably just name them key and args in the type. I don't think we need the underscore in types because they won't be marked as unused.

Markionium · 2024-08-09T06:45:45Z

compiler/crates/relay-typegen/src/visit.rs

+    let context_import = if resolver_metadata.live {
+        match &typegen_context
+            .project_config
+            .typegen_config
+            .live_resolver_context_type
+        {
+            Some(LiveResolverContextTypeInput::Path(context_import)) => {
+                Some(LiveResolverContextType {
+                    name: context_import.name.clone().intern(),
+                    import_path: typegen_context.project_config.js_module_import_identifier(
+                        &typegen_context.project_config.artifact_path_for_definition(
+                            typegen_context.definition_source_location,
+                        ),
+                        &PathBuf::from(&context_import.path),
+                    ),
+                })
+            }
+            Some(LiveResolverContextTypeInput::Package(context_import)) => {
+                Some(LiveResolverContextType {
+                    name: context_import.name.clone().intern(),
+                    import_path: context_import.package.clone().intern(),
+                })
+            }
+            None => None,
+        }
+    } else {
+        None
+    };


Since only the import_path is different we could do something like this (Note I didn't run this)

I think it doesn't matter too much but it might make it a bit clearer.

Suggested change

let context_import = if resolver_metadata.live {

match &typegen_context

.project_config

.typegen_config

.live_resolver_context_type

{

Some(LiveResolverContextTypeInput::Path(context_import)) => {

Some(LiveResolverContextType {

name: context_import.name.clone().intern(),

import_path: typegen_context.project_config.js_module_import_identifier(

&typegen_context.project_config.artifact_path_for_definition(

typegen_context.definition_source_location,

),

&PathBuf::from(&context_import.path),

),

})

}

Some(LiveResolverContextTypeInput::Package(context_import)) => {

Some(LiveResolverContextType {

name: context_import.name.clone().intern(),

import_path: context_import.package.clone().intern(),

})

}

None => None,

}

} else {

None

};

let context_import = if resolver_metadata.live {

match &typegen_context

.project_config

.typegen_config

.live_resolver_context_type

{

Some(context_type) => {

let import_path = match context_type {

LiveResolverContextTypeInput::Path(context_import) => {

typegen_context.project_config.js_module_import_identifier(

&typegen_context.project_config.artifact_path_for_definition(

typegen_context.definition_source_location,

),

&PathBuf::from(&context_import.path),

)

}

LiveResolverContextTypeInput::Package(context_import) => {

context_import.package.clone().intern()

}

};

Some(LiveResolverContextType {

name: context_type.name.clone().intern(),

import_path,

})

},

None => None,

}

} else {

None

};

I don't think this work because in the Some(LiveResolverContextType { I get an error no field nameon type&LiveResolverContextTypeInputbecause at that point the type isn't specific enough to know that both types share thename` field

Markionium · 2024-08-09T06:54:26Z

compiler/crates/relay-typegen/src/visit.rs

+    let void_type = match typegen_context.project_config.typegen_config.language {
+        TypegenLanguage::Flow | TypegenLanguage::JavaScript => AST::RawType(intern!("void")),
+        TypegenLanguage::TypeScript => AST::RawType(intern!("undefined")),
+    };
+


In javascript, we probably don't want to show any types at all? Or we could have TS docblock types, but in that case perhaps it would make more sense to align JS with TS?

Good catch. This might be caught later in the writer for JS also just by never writing any types but I'm not sure about that.

Would be good to add a test which confirms what happens here. Could be an integration test or a typegen test

alloy · 2024-08-09T18:31:49Z

packages/react-relay/__tests__/LiveResolvers-test.js

+    store: new LiveResolverStore(RelayRecordSource.create(), {
+      gcReleaseBufferSize: 0,
+      liveResolverContext: {
+        greeting: {myHello: 'Hello Allemaal!'},


@Markionium I zie you 😄

alloy · 2024-08-09T18:55:16Z

packages/relay-runtime/store/RelayReader.js

  try {
    const resolverFunctionArgs = [];
    if (field.fragment != null) {
      resolverFunctionArgs.push(fragmentKey);
+    } else {
+      // Set first argument to `null` in case we have resolver context


Nit: The code is pushing undefined, not null. What gives?

I forgot to change the comment, but I changed it to undefined to keep consistent with the : undefined we will push for the args value

alloy · 2024-08-09T19:13:09Z

packages/relay-runtime/store/RelayStoreTypes.js

+ */
+export type LiveResolverContext = {
+  [key: string]: mixed,
+};


Do we really need to insist on this being of any particular type? Unsure of the Flow semantics, perhaps this typing matches any form of JS object?

Agreed. I think context should be able to be whatever value the user wants to provide. I don't think Relay needs to have an opinion. A common case might be a Redux store.

alloy · 2024-08-09T19:14:39Z

packages/relay-runtime/store/__tests__/resolvers/HelloWorldResolverWithContext.js

+
+'use strict';
+
+import type {LiveResolverContextType} from '../../../mutations/__tests__/LiveResolverContextType';


Nit: For clarity sake, I think it would be good to rename this type to self-document that it's a test fixture representing the host's context typing, not a generic Relay provided context typing.

alloy · 2024-08-09T19:17:46Z

scripts/config.tests.json

@@ -57,7 +57,8 @@
        "relay_resolver_enable_interface_output_type": { "kind": "enabled" }
      },
      "language": "flow",
-      "experimentalEmitSemanticNullabilityTypes": true
+      "experimentalEmitSemanticNullabilityTypes": true,
+      "liveResolverContextType": { "name": "LiveResolverContextType", "path": "packages/relay-runtime/mutations/__tests__/LiveResolverContextType" }


Do we need another project that exercises the package module specifier variant?

Possibly? As far as I could find there was only one project config for all tests

alloy · 2024-08-09T19:46:23Z

yarn-error.log

@@ -0,0 +1,7442 @@
+Arguments: 


Whoopsie, accidental check-in

alloy · 2024-08-09T19:50:48Z

compiler/crates/relay-typegen/src/write.rs

@@ -178,7 +178,10 @@ pub(crate) fn write_operation_type_exports_section(
    write_import_actor_change_point(actor_change_status, writer)?;
    runtime_imports.write_runtime_imports(writer)?;
    write_fragment_imports(typegen_context, None, encountered_fragments, writer)?;
-    write_relay_resolver_imports(imported_resolvers, writer)?;
+    // TODO: Add proper support for Resolver type generation in typescript


This is follow-up work for us, right? Can you link a WI/issue?

#4772 Added

alloy · 2024-08-09T19:50:58Z

compiler/crates/relay-typegen/src/write.rs

@@ -449,7 +452,11 @@ pub(crate) fn write_fragment_type_exports_section(
    write_custom_scalar_imports(custom_scalars, writer)?;

    runtime_imports.write_runtime_imports(writer)?;
-    write_relay_resolver_imports(imported_resolvers, writer)?;
+
+    // TODO: Add proper support for Resolver type generation in typescript


#4772 Added

drewatk · 2024-08-14T21:59:41Z

@captbaritone @Markionium @alloy Ready for another look here. Addressed all comments and added some fixture tests as well. Feel free to suggest any additional tests you'd like to see.

All rust tests are passing on my local, seems to be failing on ubuntu but not mac in CI.

drewatk · 2024-08-19T16:49:51Z

@captbaritone Gentle Ping :)

captbaritone

Everything looks good! Just a few minor nits/fixups and we can land this!

captbaritone · 2024-08-20T20:52:23Z

compiler/crates/relay-typegen/src/typegen_state.rs

@@ -136,6 +136,13 @@ pub(crate) struct ImportedResolver {
    pub resolver_name: ImportedResolverName,
    pub resolver_type: AST,
    pub import_path: StringKey,
+    pub context_import: Option<LiveResolverContextType>,


Suggested change

pub context_import: Option<LiveResolverContextType>,

pub context_import: Option<ResolverContextType>,

captbaritone · 2024-08-20T20:52:36Z

compiler/crates/relay-typegen/src/typegen_state.rs

+}
+
+#[derive(Clone, Copy)]
+pub(crate) struct LiveResolverContextType {


Suggested change

pub(crate) struct LiveResolverContextType {

pub(crate) struct ResolverContextType {

captbaritone · 2024-08-20T20:54:04Z

packages/relay-runtime/store/RelayStoreSubscriptions.js

+            source,
+            snapshot.selector,
+            this._resolverCache,
+            this._resolverContext || undefined,


Suggested change

this._resolverContext || undefined,

this._resolverContext || undefined,

Suggested change

this._resolverContext || undefined,

this._resolverContext,

captbaritone · 2024-08-20T20:54:42Z

packages/relay-runtime/store/RelayStoreTypes.js

+ * `resolverContext` is set on the Relay Store.
+ * This context will be passed as the third argument to the live resolver
+ */
+export type ResolverContext = Object;


Suggested change

export type ResolverContext = Object;

export type ResolverContext = mixed;

I don't think we need to be opinionated at all as to what your context is.

captbaritone · 2024-08-20T20:56:26Z

website/docs/guides/relay-resolvers/context.md

+
+  </TabItem>
+
+  <TabItem value="Flow">


Context won't work with the Flow syntax until we do some additional work. Can you replace these Flow tabs with a note saying it's not supported for now?

captbaritone · 2024-08-20T20:57:42Z

website/docs/guides/relay-resolvers/context.md

+import Tabs from '@theme/Tabs';
+import TabItem from '@theme/TabItem';
+
+In order to pass a service, or other values to be shared with all resolvers, the `LiveResolverStore` provides a means of passing context. This gets passed to the third argument of all resolvers (live and non-live).


Maybe we could call out that this is analogous to the context argument used on the server which usually holds things like the database connection: https://graphql.org/learn/execution/#root-fields--resolvers

captbaritone · 2024-08-20T20:58:00Z

website/docs/guides/relay-resolvers/limitations.md


-In a full GraphQL implementation, resolvers would have access to a `context` and `info` argument. These arguments are not available in Relay Resolvers today. Supporting context is something we would like to do in the future, but have not yet implemented.
+In a full GraphQL implementation, resolvers would have access to an `info` argument. This argument is not available in Relay Resolvers today.


This was a fun change :)

captbaritone · 2024-08-20T20:59:08Z

compiler/crates/relay-config/src/typegen_config.rs

@@ -129,6 +148,10 @@ pub struct TypegenConfig {
    /// A map from GraphQL error name to import path, example:
    /// {"name:: "MyErrorName", "path": "../src/MyError"}
    pub custom_error_type: Option<CustomTypeImport>,
+
+    // Indicates the type to import and use as the context for live resolvers.


If you use /// here instead of // it will get picked up as part of the description in the json schema we generate. Can use that here and add similar comments to the ResolverContextTypeInput type and its variants?

captbaritone · 2024-08-20T21:00:28Z

Rust test failures on CI are unrelated. We have an ongoing issue where it is running out of disk space.

facebook-github-bot · 2024-08-20T21:03:17Z

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

captbaritone · 2024-08-21T06:03:49Z

compiler/crates/relay-compiler/relay-compiler-config-schema.json

@@ -1468,6 +1468,48 @@
          "default": false,
          "type": "boolean"
        },
+        "resolverContextType": {


If you rerun the Rust tests with the flag to update snapshots this should update to include the description

captbaritone · 2024-08-21T06:05:43Z

compiler/crates/relay-config/src/typegen_config.rs

+#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]
+pub struct ResolverContextTypeInputPath {
+    pub name: StringKey,
+    pub path: PathBuf,
+}


Suggested change

#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]

pub struct ResolverContextTypeInputPath {

pub name: StringKey,

pub path: PathBuf,

}

/// Specifies how Relay can import the Resolver context type from a path

#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]

pub struct ResolverContextTypeInputPath {

/// The name under which the type is exported from the module

pub name: StringKey,

/// The path to the module

pub path: PathBuf,

}

Can we also clarify what the path is relative to?

captbaritone · 2024-08-21T06:06:38Z

compiler/crates/relay-config/src/typegen_config.rs

+#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]
+pub struct ResolverContextTypeInputPackage {
+    pub name: StringKey,
+    pub package: StringKey,
+}


Suggested change

#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]

pub struct ResolverContextTypeInputPackage {

pub name: StringKey,

pub package: StringKey,

}

/// Specifies how Relay can import the Resolver context type from a named package

#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema)]

pub struct ResolverContextTypeInputPackage {

/// The name under which the type is exported from the package

pub name: StringKey,

/// The name of the package

pub package: StringKey,

}

captbaritone · 2024-08-21T06:07:11Z

compiler/crates/relay-typegen/src/visit.rs

                value: AST::RawType(format!("{fragment_name}$key").intern()),
                read_only: false,
                optional: false,
            });
        }
+    } else {
+        resolver_arguments.push(KeyValuePairProp {
+            key: "key".intern(),


Suggested change

key: "key".intern(),

key: intern!("key".intern),

captbaritone · 2024-08-26T16:46:27Z

compiler/crates/relay-config/src/typegen_config.rs

+    Package(ResolverContextTypeInputPackage),
+}
+
+/// Specifies how Relay can import the Resolver context type from a path


facebook-github-bot · 2024-08-26T16:50:51Z

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

captbaritone · 2024-08-27T20:04:58Z

When trying to land this internally I found that it was going to be very difficult to rollout this change safely if we change how resolvers that don't read a parent object or rootFragment get called. So, I've updated the diff a bit internally to go back to the previous behavior where the first argument is omitted if there is no rootFragment and the resolver is not on a model type. I've updated the docs to match.

facebook-github-bot · 2024-09-04T00:12:09Z

@captbaritone merged this pull request in 2973a0f.

Markionium mentioned this pull request Jun 4, 2024

Provided variables and Live Resolver modules should allow injected dependencies #4000

Open

Markionium force-pushed the resolver-context branch from ce6d88f to 9bd0d55 Compare June 4, 2024 13:44

drewatk mentioned this pull request Jul 16, 2024

Remove @ts-nocheck from generated Typescript files #4745

Open

Markionium added 3 commits July 23, 2024 20:57

Add context to relay resolvers

8c31211

start fixing some stuff

eefbfbc

Rework resolvercontext based on feedback

62a5c7f

Markionium force-pushed the resolver-context branch from 9bd0d55 to 62a5c7f Compare July 31, 2024 18:03

drewatk force-pushed the resolver-context branch from 49699d5 to fa4660d Compare July 31, 2024 21:16

fix: flow errors

8f8eb58

drewatk force-pushed the resolver-context branch from fa4660d to 8f8eb58 Compare July 31, 2024 21:30

drewatk added 4 commits August 1, 2024 16:55

wip: context args typing

72b6c91

typegen changes for liveResolverContextType, tests to go

e0c8311

Update JS tests

1022eba

fix: remove todo comments

837ef8c

Markionium marked this pull request as ready for review August 8, 2024 18:48

facebook-github-bot added the CLA Signed label Aug 8, 2024

Markionium commented Aug 9, 2024

View reviewed changes

alloy reviewed Aug 9, 2024

View reviewed changes

yarn-error.log Outdated

@@ -0,0 +1,7442 @@

Arguments:

Copy link

Contributor

alloy Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoopsie, accidental check-in

drewatk reacted with thumbs up emoji

alloy reviewed Aug 9, 2024

View reviewed changes

Merge remote-tracking branch 'facebook/main' into resolver-context

b0fac59

drewatk mentioned this pull request Aug 14, 2024

Typescript Validation for Resolver Types #4772

Closed

drewatk added 5 commits August 14, 2024 10:37

add link to issue

816b5ea

remove "live" from the naming of the context and types

b3d492f

fix: lint

520dc93

fix: rust lint

0ab2818

add new fixture tests

24e8e6c

drewatk added 2 commits August 14, 2024 15:09

test: add a JS fixture test for when type is defined it will be ignored

5818987

Merge remote-tracking branch 'facebook/main' into resolver-context

78cabf0

captbaritone requested changes Aug 20, 2024

View reviewed changes

drewatk added 2 commits August 20, 2024 14:14

PR suggestions

8d8e83a

fix: type errors

5963c6f

captbaritone requested changes Aug 21, 2024

View reviewed changes

drewatk added 6 commits August 21, 2024 09:02

fix: typegen config doc comments/schema

bf773ec

fix: intern

7520e4b

fix: comment

1bfd08f

fix: schema

5edfef9

Merge branch 'main' into resolver-context

165ce8f

Merge branch 'main' into resolver-context

2913709

captbaritone approved these changes Aug 26, 2024

View reviewed changes

facebook-github-bot closed this in 2973a0f Sep 4, 2024

facebook-github-bot added the Merged label Sep 4, 2024


		'use strict';

		import type {LiveResolverContextType} from '../../../mutations/__tests__/LiveResolverContextType';

	pub context_import: Option<LiveResolverContextType>,
	pub context_import: Option<ResolverContextType>,

	pub(crate) struct LiveResolverContextType {
	pub(crate) struct ResolverContextType {

	this._resolverContext \|\| undefined,
	this._resolverContext \|\| undefined,

	export type ResolverContext = Object;
	export type ResolverContext = mixed;


		In a full GraphQL implementation, resolvers would have access to a `context` and `info` argument. These arguments are not available in Relay Resolvers today. Supporting context is something we would like to do in the future, but have not yet implemented.
		In a full GraphQL implementation, resolvers would have access to an `info` argument. This argument is not available in Relay Resolvers today.

Add context to relay resolvers #4704

Add context to relay resolvers #4704

Conversation

Markionium commented Jun 4, 2024

facebook-github-bot commented Jun 4, 2024

Action Required

Process

captbaritone commented Jun 4, 2024

captbaritone commented Jun 12, 2024

captbaritone commented Aug 1, 2024

drewatk commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alloy Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewatk commented Aug 14, 2024 • edited Loading

drewatk commented Aug 19, 2024

captbaritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captbaritone commented Aug 20, 2024

facebook-github-bot commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 26, 2024

captbaritone commented Aug 27, 2024

facebook-github-bot commented Sep 4, 2024

alloy Aug 9, 2024 •

edited

Loading

drewatk commented Aug 14, 2024 •

edited

Loading