Allow `Node` interface `id` field to be configurable. by kesne · Pull Request #3638 · facebook/relay

kesne · 2021-11-13T00:30:07Z

This adds a new option to the Rust compiler, nodeInterfaceIdField, which allows customization of the Node interface's id field. I initially tried to determine this via inference, but it's entirely possible that the Node interface has multiple id fields, so I figured explicitly specifying it made more sense.

orta · 2021-12-11T09:42:09Z

( +1-ing the desire for this, lots of OSS JS/graphQL tooling takes over id or makes it hard to get the right table to query for an id. At Artsy there was so much interacting with legacy systems we had id (table), _id(db) and then __id which was finally the one we could use for GQL object identification )

maraisr · 2021-12-11T10:23:42Z

We'd also need to let the relay-runtime know about that property, as so much hangs on that ID field 😅

kesne · 2021-12-14T18:28:53Z

@maraisr From what I can tell, getDataID should be able to be used to accomplish that:

relay/packages/relay-runtime/store/RelayModernEnvironment.js

Line 79 in 38f1c96

+getDataID?: ?GetDataID,

maraisr · 2021-12-14T21:53:35Z

Just a ripe ol pain, coz in places where there is no id field, relay still tries to normalise like serialising some input arguments and such. I've not looked too close, but would suspect this method to have to implement all of those avenues as well?

facebook-github-bot · 2021-12-15T17:48:59Z

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

alunyov

Thanks for adding this! Sorry, we dropped the ball on this PR and did not review it earlier.

The main feedback is that let's try re-using FeatureFlags for this id field configuration.

alunyov · 2021-12-15T21:43:58Z

compiler/crates/relay-compiler/src/compiler_state.rs

    }

-    fn is_change_safe(&self, sources: &SchemaSources) -> bool {
+    fn is_change_safe(&self, sources: &SchemaSources, project_config: &ProjectConfig) -> bool {


can we add node_interface_id_field to the compiler_state itself?

I tried to do this, but the config that's used to create the compiler_state is the root config, not the project config, so this seemed challenging to try to move.

alunyov · 2021-12-15T21:44:23Z

compiler/crates/relay-compiler/src/compiler_state.rs

                &Vec::<(&str, SourceLocationKey)>::new(),
            ) {
-                Ok(schema) => schema_change.is_safe(&schema),
+                Ok(schema) => schema_change.is_safe(&schema, project_config.node_interface_id_field),


then here we can pass self. node_interface_id_field

alunyov · 2021-12-15T21:45:38Z

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

    js_module_format: JsModuleFormat,
+
+    #[serde(default)]
+    pub node_interface_id_field: Option<StringKey>,


can we add comment here, and also to the compiler README.md that explains the purpose of this field

Also, I think this is something from the internal discussion about this PR.

I think we should add this field to FeatureFlags here:
https://github.com/facebook/relay/blob/59c9bc01c1d215b75d6c683f6ee1074a8110fd7b/compiler/crates/common/src/feature_flags.rs

And this PR (or this can be a separate PR) should just add feature_flags to SingleProjectConfigFile.

I can make the change to move this to feature flags. I didn't originally since it seems like all of the feature flag config was resolved to boolean values, whereas this is a string configuration value, which didn't seem to me like a feature flag.

this is a string configuration value, which didn't seem to me like a feature flag.

Fair point.

I general, I think this field is similar with ConnectionInterface. I think both of these should be defined on the project_config here:

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

Lines 252 to 276 in 59c9bc0

let project_config = ProjectConfig {

name: project_name,

base: config_file_project.base,

enabled: true,

schema_extensions: config_file_project.schema_extensions,

output: config_file_project.output,

extra_artifacts_output: config_file_project.extra_artifacts_output,

shard_output: config_file_project.shard_output,

shard_strip_regex,

schema_location,

typegen_config: config_file_project.typegen_config,

persist: config_file_project.persist,

variable_names_comment: config_file_project.variable_names_comment,

extra: config_file_project.extra,

test_path_regex,

feature_flags: Arc::new(

config_file_project

.feature_flags

.unwrap_or_else(|| config_file_feature_flags.clone()),

),

filename_for_artifact: None,

skip_types_for_artifact: None,

rollout: config_file_project.rollout,

js_module_format: config_file_project.js_module_format,

};

Something like schema_configuration field which is a struct:

struct SchemaConfiguration { connection_interface: ConnectionInterface, node_interface_id_field: Option<StringKey>, }

But this refactoring can be done independently from this PR.

or schema_config for short, as we already have typegen_config.

@alunyov I moved it to a FeatureFlag, just so you can see what that would be like. Let me know which approach you'd like to move forward with!

Thanks @kesne! I importing this internally.

alunyov · 2021-12-15T21:50:53Z

compiler/crates/relay-transforms/src/apply_transforms.rs

@@ -130,6 +134,7 @@ fn apply_common_transforms(
    program: Arc<Program>,
    connection_interface: &ConnectionInterface,
    feature_flags: Arc<FeatureFlags>,


the benefits of adding node_interface_id_field to FeatureFlags is that you already have them in apply_transforms so it's less changes.

facebook-github-bot · 2021-12-15T23:28:48Z

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

alunyov · 2021-12-16T17:17:18Z

So, moving this to feature_flags didn't make it cleaner.

And I totally agree with you @kesne that it doesn't seem right to have these settings in the feature_flags. All these suggestions were with the idea that we somehow add this id_field_name without passing this individual property around too much.

One problem was, we could not use ProjectConfig type in the relay-transforms, as it was defined in the relay-compiler crate. I've moved ProjectConfig to a separate crate so we can use this type both in relay-compiler and in relay-transforms. See: 5baad8c

I'm working on updates to apply_transform that will make them accept project_config, these change should land soon.

I will also add new property on the ProjectConfig - schema_config where we can put node_interface_id_field.

alunyov · 2021-12-17T14:09:01Z

The changes to the project_config landed. I think we can add the id_field_name to the schema_config here:

https://github.com/facebook/relay/blob/main/compiler/crates/relay-config/src/project_config.rs#L42-L44

alunyov · 2021-12-17T16:01:53Z

A quick update. I think there will be another iteration on top of the schema_config. We need it also here: #3121 (comment) - so I think I'll add it directly to program: it would be easier to access these configs in various transforms.

Summary: A follow-up for this PR: #3638 Pass schema configuration to `relay-typegen` and use the `id` name for the config. Reviewed By: kassens Differential Revision: D33479397 fbshipit-source-id: 617d341609c0749ca2e6921b74a6354508f70455

Lalitha-Iyer · 2022-06-25T00:16:33Z

@alunyov There could be pagination bug Related to this pr. If we configure the compiler to use a custom id, I found pagination loadNext fails at runtime, as it queries with node.id and not the custom configured node id.
Looking at the source we probably need to change paginationVariables.id to paginationVariables.identifierField
https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/useLoadMoreFunction.js#L212

Let me know if I should have a more sandboxed example and a new issue.

Emilios1995 · 2022-08-09T18:19:20Z

@alunyov There could be pagination bug Related to this pr. If we configure the compiler to use a custom id, I found pagination loadNext fails at runtime, as it queries with node.id and not the custom configured node id. Looking at the source we probably need to change paginationVariables.id to paginationVariables.identifierField https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/useLoadMoreFunction.js#L212

Let me know if I should have a more sandboxed example and a new issue.

@Lalitha-Iyer I opened an issue for this already. Sadly I haven't had time to debug and the issue hasn't received much attention.

Lalitha-Iyer · 2022-08-15T22:56:45Z

@Emilios1995 Appreciate you opening an issue. I will follow along.

facebook-github-bot added the CLA Signed label Nov 13, 2021

alunyov requested changes Dec 15, 2021

View reviewed changes

kesne and others added 5 commits December 15, 2021 17:29

Update relay compiler to support node _id field

a475bce

Mostly working configurable node interface ID

6ddf162

Wire through the rest of the id fields

71f921d

Remove changes to test project

9539251

Fix bad rebase

6d2b0d8

kesne force-pushed the node-id-config branch from a4b3c79 to 6d2b0d8 Compare December 15, 2021 22:37

Migrate to using FeatureFlags for configuration

323c999

Fix tests

ae8a1bc

facebook-github-bot closed this in c3f0292 Jan 5, 2022

Emilios1995 mentioned this pull request Aug 30, 2022

Fix refetching and pagination when using a custom nodeInterfaceIdField #4053

Closed

	let project_config = ProjectConfig {
	name: project_name,
	base: config_file_project.base,
	enabled: true,
	schema_extensions: config_file_project.schema_extensions,
	output: config_file_project.output,
	extra_artifacts_output: config_file_project.extra_artifacts_output,
	shard_output: config_file_project.shard_output,
	shard_strip_regex,
	schema_location,
	typegen_config: config_file_project.typegen_config,
	persist: config_file_project.persist,
	variable_names_comment: config_file_project.variable_names_comment,
	extra: config_file_project.extra,
	test_path_regex,
	feature_flags: Arc::new(
	config_file_project
	.feature_flags
	.unwrap_or_else(\|\| config_file_feature_flags.clone()),
	),
	filename_for_artifact: None,
	skip_types_for_artifact: None,
	rollout: config_file_project.rollout,
	js_module_format: config_file_project.js_module_format,
	};

Conversation

kesne commented Nov 13, 2021

Uh oh!

orta commented Dec 11, 2021

Uh oh!

maraisr commented Dec 11, 2021

Uh oh!

kesne commented Dec 14, 2021

Uh oh!

maraisr commented Dec 14, 2021

Uh oh!

facebook-github-bot commented Dec 15, 2021

Uh oh!

alunyov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 15, 2021

Uh oh!

alunyov commented Dec 16, 2021

Uh oh!

alunyov commented Dec 17, 2021

Uh oh!

alunyov commented Dec 17, 2021

Uh oh!

Lalitha-Iyer commented Jun 25, 2022

Uh oh!

Emilios1995 commented Aug 9, 2022

Uh oh!

Lalitha-Iyer commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants