Modifying @relay_test_operation to write metadata for client fields

[stewartavery]

Problem

There were recent changes to allow support for mocking client side fields with MockPayloadGenerator. Now that these fields can be used we need to make sure @relay_test_operation writes associated metadata for them during artifact generation.

Solution

Add a new FeatureFlag, enable_mock_client_data_metadata, to control whether or not client data gets extra metadata generated when @relay_test_operation is applied to a query.

Then, modify the !field.is_extension checks in both ScalarField and LinkedField patterns to also look for the feature flag value before skipping client data.

Finally, add new test cases to support the new variations with client side schema fields. The mod.rs file was changed to create a local schema instance that incorporates client side schema extensions.

[stewartavery]

I referenced other mod.rs files in order to get this working

[voideanvalue]

What's the rationale for doing this conditionally with a FeatureFlag instead of doing this unconditionally?

[stewartavery]

@voideanvalue

What's the rationale for doing this conditionally with a FeatureFlag instead of doing this unconditionally?

It was along the same line as the feedback I got here in a previous PR with client field changes. The metadata exposed to MockPayloadGenerator is now more rich for client fields — this could break systems(like snapshot tests or other assertions) that implicitly depend on the behavior today.

I was thinking the flag could be removed in the next major version of Relay and at that point we could make this behavior the new default!

[stewartavery]

this is the new behavior with the feature flag enabled. This extra metadata will allow MockPayloadGenerator to auto-mock client fields when they're not provided in the associated resolver

[voideanvalue]

@stewartavery

If we don't add a feature flag and do this unconditionally... all that'd do is add additional metadata about to the generated artifact for any existing operations that have the @relay_test_operation directive and includes client fields.

That in itself isn't something I'd consider a breaking change.

The output from calling RelayMockPayloadGenerator.generate on any such operation will only change the output if mockClientData option added in 8a44673 is used. I doubt anyone is already using the mockClientData option. I'd also argue that this is finishing an incomplete feature and not breaking existing behavior... specially since the last relay release was before 8a44673 was committed.

That's why I find the FeatureFlag to be an overkill here.

Am I missing something?

[stewartavery]

@voideanvalue

The output from calling RelayMockPayloadGenerator.generate on any such operation will only change the output if mockClientData option added in 8a44673 is used

I'm not sure that's true since my last commit only gave support for providing manual client data to a resolver. I never made any changes to test_operation_metadata. So technically someone could've been using a client field in their production code but just not relying on MockPayloadGenerator to make any assertions against it within their tests.

But now with this latest changes those artifacts will update will update at compile time — which doesn't care about what options are passed to MockPayloadGenerator since that's a runtime thing.

Edit: reading your comment more closely it seems that you realize this 🤦. If you think it doesn't count as a breaking change I'm happy to remove the feature flag logic!

[voideanvalue]

Yes the generated artifacts will get changed.

relay/packages/relay-test-utils/RelayMockPayloadGenerator.js

Line 900 in 7a12c67

operation.request.node.params.metadata?.relayTestingSelectionTypeInfo;

this is the only place we ever read relayTestingSelectionTypeInfo

If I'm reading RelayMockPayloadGenerator correctly... any additional selections that will get added to relayTestingSelectionTypeInfo in generated artifacts after this change... will not be traversed unless mockClientData option is true.

So the only cases where runtime behavior changes is when RelayMockPayloadGenerator is explicitly passed {mockClientData: true}.

mockClientData was added in 8a44673. the latest relay release predates that commit. even if this was a breaking change relative to 8a44673, it's not a breaking change relative to the latest release.

(it's possible I'm being oblivious to something that you're seeing. so please do call me out on anything I'm asserting incorrectly or overlooking.)

[stewartavery]

@voideanvalue nope that's correct! I had in mind the hyrum's law feedback from the last PR. But yeah I can update things next week 👍

[stewartavery]

@voideanvalue just updated!

[alunyov]

Thank you!

[facebook-github-bot]

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