Fragment Arguments Reference Implementation #3835

mjmahone · 2023-01-19T18:23:35Z

This implements Fragment Arguments, as described by the spec changes in graphql/graphql-spec#1010.

We have:

Updates AST to support new syntax of arguments on fragment spreads, and updating our legacy "experimental" fragment variables definition syntax to be fully supported as a way of defining the names and types for the arguments passed to fragment spreads.
Updated Validation (including Fields Can Merge)
Updated the Executor.

I am creating a clean commit rather than updating #3152, as the discussion from the previous PR was confusing newcomers with old historical details that we've decided to abandon.

Please use this PR for reviewing and verifying the accuracy of the PR, with inline comments when possible.

For discussions about Fragment Arguments as an idea, missing features or open questions, please open or respond to a thread in the graphql-wg discussion space: graphql/graphql-wg#1239

netlify · 2023-01-19T18:23:40Z

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	`650a99f`
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63e5680f2248480008b036a7
😎 Deploy Preview	https://deploy-preview-3835--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2023-01-19T18:25:57Z

Hi @mjmahone, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

@github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
@github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

github-actions · 2023-02-08T01:38:11Z

@github-actions run-benchmark

@mjmahone Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/4118969873/jobs/7112119320#step:6:1

src/language/parser.ts

leebyron

This is really exciting! Nuanced behavior here that's found a sweet spot of value.

I dropped in a mix of tactical and high level feedback.

The one overarching piece of feedback is to be careful with naming things to get consistency. This adds both "fragment variable definitions" and "fragment spread arguments". I don't have a strong opinion about what we call the whole RFC'd feature, but we should be consistent about calling the things you provide to fragment spreads "arguments" and the things you define at the top of a fragment "variable definitions". I noticed some swapping of this terminology in a confusing way, as well as a few others like "local" vs "fragment".

leebyron · 2023-02-09T05:08:22Z

src/__testUtils__/kitchenSinkQuery.ts

@@ -10,7 +10,7 @@ query queryName($foo: ComplexType, $site: Site = MOBILE) @onQuery {
          ...frag @onFragmentSpread
        }
      }
-


leebyron · 2023-02-09T05:33:38Z

src/utilities/keyForFragmentSpread.ts

+  const printedArguments: Array<string> = fragmentArguments
+    .map(print)
+    .sort((a, b) => a.localeCompare(b));
+  return fragmentName + '(' + printedArguments.join(',') + ')';


What are the tests which exercise these keys being skipped correctly?

I imagine this wants to allow the following to not skip over the second call to Frag with different arguments

{ ...Frag(arg: "A") ...Frag(arg: "B") }

Should this just be strictly disallowed in the first place similar to how repeated fields need to have equivalent arguments?

Or if we do allow this, it seems like there might be examples where this would cause two calls to a fragment that are visually similar but do different things to produce the wrong result?

query ($var: Boolean = false) { ...AorB(which: $var) ...Shadow } fragment AorB($which: Boolean) { a: echo(s: "A") @include(if: $which) b: echo(s: "B") @skip(unless: $which) } fragment Shadow($var: Boolean = true) { ...AorB(which: $var) }

By proposed spec I'd this to produce:

{ a: "A", b: "B", }

However I think this keying algorithm will produce AorB(which: $var) as the key for both, and they're used within the same selection set, so same sweep of CollectFields, so I think the second would be skipped over and instead produce:

{ a: "A", }

Is this right? If so, then we'd need to know the result of fragmentArgumentSubstitutions as an input to determining this key.

I see you're using these keys to identify fragments during validation as well, which is before we have runtime values, so maybe there's something to be done to swap in the argument values (which may be variables themselves) to ensure these keys represent the correctly unique sources of runtime values in that case?

In that case the first instance of AorB should key to AorB(which: $var) since the runtime operation variable $var is what would provide the source, and the second would be AorB(which: true) since the fragment variable's default value got applied since no argument was provided. Now they're different and you'll see both during collectFields.

However at runtime, there is a chance that the $var is provided the value true then since you know that information during execution you could skip the second one - it would be okay if they keyed the same.

Should this just be strictly disallowed in the first place similar to how repeated fields need to have equivalent arguments?

Yes, IMO: we should be treating fragment spreads with the same name exactly like fields with the same name that need to be able to merge. If we don't it greatly increases the complexity of field merging validation, and likely of the executor.

If you're only changing a variable that isn't actually used within one spread's context, technically the executor ought to be able to handle it. But I think the devex is worse to allow that case than to just make it invalid.

If so, then we'd need to know the result of fragmentArgumentSubstitutions as an input to determining this key. ...AorB discussion...

This is a good test case to add for OverlappingFieldsCanBeMergedRule, and it should fail (but likely doesn't as-is, great catch!). I think we might need to add a local-variable-scope indicator to the key for validation (though at execution time we should always have actualized values). Good call that the keying function can't quite be shared as-is.

Oh! I really like this. It's constraining, but in a way that's far more simplifying than restricting. I think if you apply this then you don't need "fragment key" as a concept at all!

I imagine the validation rule either akin to or part of OverlappingFieldsCanBeMergedRule which ensures any two fragment spreads of the same name in the same scope would always produce exactly the same set of fragment variable values (aka: always has the same source of values). I think most of the logic you added is approaching that, but I had just misunderstood that.

In that case, you can assume that for a valid query you only need to spread the first instance of any fragment with a given name since the above rule guarantees that any fragment you encounter later with the same name would provide identical results.

Because you can assume that, you should be able to just keep the existing logic of using the fragment name instead of computing a key during collectFields

leebyron · 2023-02-09T05:39:12Z

src/execution/collectFields.ts

          );
        }
        break;
      }
      case Kind.FRAGMENT_SPREAD: {
-        const fragName = selection.name.value;
+        const fragmentKey = keyForFragmentSpread(selection);


Nitpick feedback: lets move this closer to where its used so the earlier short-circuits can be checked first.

Could this be a potential performance concern since collectFields is a highly called function? Is there any opportunity to compute these keys lazily - only doing so if there's first a simple name collision?

It's likely worth testing the performance benchmarks by making this lazy, but my suspicion is it will be unlikely to make a difference in most cases and/or the complexity of keeping an additional layer of cache (today it's key => fragment, after it would be name => (first fragment, spreads-to-merge, computed-keys-if-multiple-spreads => fragment)).

Because we don't have a real scope, we do want to cache the spreads' fragment result, as we could hit ...profilePicture(size: 30) many times and we don't really want to visit profilePicture and replace $size each time.

+100 - I got too far into solutioning here and you are correct that we should try to measure some degenerate cases to understand it first. See the other thread about validation which might side-step all of this by removing the need for it in the first place

leebyron · 2023-02-09T05:41:57Z

src/language/kinds.ts

@@ -22,6 +22,7 @@ export enum Kind {
  FRAGMENT_SPREAD = 'FragmentSpread',
  INLINE_FRAGMENT = 'InlineFragment',
  FRAGMENT_DEFINITION = 'FragmentDefinition',
+  FRAGMENT_ARGUMENT_DEFINITION = 'FragmentArgumentDefinition',


I don't see this used elsewhere

Oooh sorry leftover from when I had a new AST node for the variable defs.

leebyron · 2023-02-09T05:46:34Z

src/language/printer.ts

+      let argsLine = prefix + wrap('(', join(args, ', '), ')');
+
+      if (argsLine.length > MAX_LINE_LENGTH) {
+        argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)');
+      }


I think this is the same as Field: opportunity to capture the repeat logic in a function at the bottom of the file?

leebyron · 2023-02-09T05:59:45Z

src/utilities/substituteFragmentArguments.ts

+  return visit(def.selectionSet, {
+    Variable(node) {
+      return argumentValues[node.name.value];
+    },
+  });


I thought the draft spec behavior was to introduce limited scope such that during collection you create a new set of variable definitions for these fragment spreads, rather than having to do a visit/replace.

I'm a bit worried about this visit creating a performance issue, but more importantly that these edits could potentially lead to confusingly printed error messages if an error occurred at a field/arg/variable level where the visited replacement was printed rather than the original.

There may be other options, but the alternative I can imagine is that fragmentVariableValues has to be collected alongside fields as they are collected such that during execution you have access to it

I flip-flopped. The current spec draft uses the replacement algorithm because adding actual scope infected the algorithms a lot more. The problem is you need to basically replace type VariableDefinitions = Map<string, [VariableDefinition, VariableValue]> everywhere with type VariableScope = { localVariables: VariableDefinitions, operationVariables: VariableDefinitions }. It's certainly worth trying to get the scoped version out before shipping the spec change, but it's not the smallest change possible, which is likely to lead to some mistakes.

Whereas if we can get all the test cases described, working and solidified with the smaller-change replacement option, we'll have a lot of confidence in switching to scoped variables.

Fair enough. That makes me think we should call this "Fragment Templates" and mirror some of the C++ nomenclature. I don't think I like it, but it's closer to what we're describing :)

leebyron · 2023-02-09T06:12:11Z

src/utilities/substituteFragmentArguments.ts

+      // To remove this hack, we need to either:
+      //    - include fragment argument scope when evaluating fields
+      //    - make unset fragment arguments invalid
+      // Requiring the spread to pass all non-default-defined arguments is nice,
+      // but makes field argument default values impossible to use.
+      substitutions[argumentName] = {
+        kind: Kind.VARIABLE,
+        name: { kind: Kind.NAME, value: '__UNSET' },
+      };


// To remove this hack, we need to either: // - include fragment argument scope when evaluating fields

I think probably yes, that. Is that too crazy? This hack is not ideal.

Also, query ($__UNSET: String = "SomeDefault") {... is valid and while most query authors wouldn't write this, this seems like some kind of security vector or something

Oh I should just replace it with $0__UNSET and then it's not valid :)

Is that too crazy?

I spent a week or so of full-time work trying to get the spec algorithms and graphql-js to have full scope. It was just a much more pervasive change. It's not too crazy, but the replacement strategy is certainly a smaller change for both.

would an equivalent behavior be to also visit each InputValue on leave and if its unset to remove it? In other words - rather than putting in a hacked variable AST node, leave the AST that would be the equivalent to an unset var

leebyron · 2023-02-09T06:13:03Z

src/validation/ValidationContext.ts

@@ -199,16 +202,24 @@ export class ValidationContext extends ASTValidationContext {
    let usages = this._variableUsages.get(node);
    if (!usages) {
      const newUsages: Array<VariableUsage> = [];
-      const typeInfo = new TypeInfo(this._schema);
+      const typeInfo = this._typeInfo;
+      const localVariableDefinitions =


fragmentVariableDefinitions

leebyron · 2023-02-09T06:14:34Z

src/validation/__tests__/KnownDirectivesRule-test.ts

@@ -219,9 +222,13 @@ describe('Validate: Known directives', () => {
        message: 'Directive "@onQuery" may not be used on FIELD.',
        locations: [{ column: 19, line: 16 }],
      },
+      {
+        message: 'Directive "@onField" may not be used on VARIABLE_DEFINITION.',


Not blocking for this PR, but we may want to consider this directive location name being different - maybe to FRAGMENT_VARIABLE_DEFINITION?

Yeah it's weird: if they're their own AST node then absolutely. If not... maybe? I think probably?

This is a small change and another thing I flip-flopped on.

DirectiveLocations and AST nodes are not a perfect 1:1

See getDirectiveLocationForASTPath() for the mapping, there's already a few bits of logic there to consider extending in a similar way.

I think keeping the AST minimally changed is great, so it makes sense to me to have the VARIABLE_DEFINITION AST node map to one of two directive locations based on the parent AST node being a fragment or not.

leebyron · 2023-02-09T06:19:35Z

src/validation/rules/NoUnusedVariablesRule.ts

      const usages = context.getRecursiveVariableUsages(operation);
      const variableNameUsed = new Set<string>(
-        usages.map(({ node }) => node.name.value),
+        usages
+          // Skip variables used as fragment arguments
+          .filter(({ fragmentVarDef }) => !fragmentVarDef)
+          .map(({ node }) => node.name.value),
      );


I noticed these get skipped over in NoUndefinedFragmentVariableRule, maybe the better place to edit is changing getRecursiveVariableUsages() to something like getOperationVariableUsages()?

If we're OK making a breaking change. But Meta has custom validation rules that use getRecursiveVariableUsages so I imagine other people likely do too?

Or leave the existing method behind (optionally deprecate it if its not used in core anymore?) and introduce this new one?

mjmahone

Given our discussion at the APAC WG: if we're going to have a flag, I think it makes sense to split this PR into a series of dependent PRs. We can probably also pair graphql-spec PRs with each:

AST + parse + print changes (I don't think there's much controversy on syntax, maybe just directive location being the one thing)
- Validation rule changes (a little more controversial, specifically with how shadowing must work)
- Executor changes

This sequencing would help us unblock the tooling changes we need while still making forward progress on the gnarlier reviews and choices.

mjmahone · 2023-02-09T06:46:10Z

src/execution/collectFields.ts

          );
        }
        break;
      }
      case Kind.FRAGMENT_SPREAD: {
-        const fragName = selection.name.value;
+        const fragmentKey = keyForFragmentSpread(selection);


It's likely worth testing the performance benchmarks by making this lazy, but my suspicion is it will be unlikely to make a difference in most cases and/or the complexity of keeping an additional layer of cache (today it's key => fragment, after it would be name => (first fragment, spreads-to-merge, computed-keys-if-multiple-spreads => fragment)).

Because we don't have a real scope, we do want to cache the spreads' fragment result, as we could hit ...profilePicture(size: 30) many times and we don't really want to visit profilePicture and replace $size each time.

mjmahone · 2023-02-09T06:47:30Z

src/language/kinds.ts

@@ -22,6 +22,7 @@ export enum Kind {
  FRAGMENT_SPREAD = 'FragmentSpread',
  INLINE_FRAGMENT = 'InlineFragment',
  FRAGMENT_DEFINITION = 'FragmentDefinition',
+  FRAGMENT_ARGUMENT_DEFINITION = 'FragmentArgumentDefinition',


Oooh sorry leftover from when I had a new AST node for the variable defs.

mjmahone · 2023-02-09T06:54:07Z

src/utilities/TypeInfo.ts

+              // Minor hack: transform the FragmentArgDef
+              // into a schema Argument definition, to
+              // enable visiting identically to field/directive args


I am not 100% sure this really is a hack: it's semantically correct, in that we are inside a GraphQLArgument, it's just that argument is not defined by the schema but by some other definition. It's not really different from defining a type extension in the same document you use it.

What would be nicer would be if we had a concept of fragment signatures: that's kind of what I'm doing in my first-pass loop over all definitions. The signatures would probably be composed of GraphQLSchema pieces, in which case we'd have a GraphQLArgument without the "upcasting" from AST here. This is what Relay does in their IR.

mjmahone · 2023-02-09T07:05:14Z

src/utilities/keyForFragmentSpread.ts

+  const printedArguments: Array<string> = fragmentArguments
+    .map(print)
+    .sort((a, b) => a.localeCompare(b));
+  return fragmentName + '(' + printedArguments.join(',') + ')';


Should this just be strictly disallowed in the first place similar to how repeated fields need to have equivalent arguments?

Yes, IMO: we should be treating fragment spreads with the same name exactly like fields with the same name that need to be able to merge. If we don't it greatly increases the complexity of field merging validation, and likely of the executor.

If you're only changing a variable that isn't actually used within one spread's context, technically the executor ought to be able to handle it. But I think the devex is worse to allow that case than to just make it invalid.

If so, then we'd need to know the result of fragmentArgumentSubstitutions as an input to determining this key. ...AorB discussion...

This is a good test case to add for OverlappingFieldsCanBeMergedRule, and it should fail (but likely doesn't as-is, great catch!). I think we might need to add a local-variable-scope indicator to the key for validation (though at execution time we should always have actualized values). Good call that the keying function can't quite be shared as-is.

mjmahone · 2023-02-09T07:07:57Z

src/utilities/substituteFragmentArguments.ts

+  def: FragmentDefinitionNode,
+  fragmentSpread: FragmentSpreadNode,
+): SelectionSetNode {
+  const argumentDefinitions = def.variableDefinitions;


Agree it's not super clean. I likely need to do another pass but there is some logic here: argumentDefinitions are the definitions for the arguments we need to look up based on the spread, not the variable definitions that the spread currently has access to.

mjmahone · 2023-02-09T07:13:41Z

src/utilities/substituteFragmentArguments.ts

+  return visit(def.selectionSet, {
+    Variable(node) {
+      return argumentValues[node.name.value];
+    },
+  });


I flip-flopped. The current spec draft uses the replacement algorithm because adding actual scope infected the algorithms a lot more. The problem is you need to basically replace type VariableDefinitions = Map<string, [VariableDefinition, VariableValue]> everywhere with type VariableScope = { localVariables: VariableDefinitions, operationVariables: VariableDefinitions }. It's certainly worth trying to get the scoped version out before shipping the spec change, but it's not the smallest change possible, which is likely to lead to some mistakes.

Whereas if we can get all the test cases described, working and solidified with the smaller-change replacement option, we'll have a lot of confidence in switching to scoped variables.

mjmahone · 2023-02-09T07:16:29Z

src/utilities/substituteFragmentArguments.ts

+      // To remove this hack, we need to either:
+      //    - include fragment argument scope when evaluating fields
+      //    - make unset fragment arguments invalid
+      // Requiring the spread to pass all non-default-defined arguments is nice,
+      // but makes field argument default values impossible to use.
+      substitutions[argumentName] = {
+        kind: Kind.VARIABLE,
+        name: { kind: Kind.NAME, value: '__UNSET' },
+      };


Oh I should just replace it with $0__UNSET and then it's not valid :)

Is that too crazy?

I spent a week or so of full-time work trying to get the spec algorithms and graphql-js to have full scope. It was just a much more pervasive change. It's not too crazy, but the replacement strategy is certainly a smaller change for both.

mjmahone · 2023-02-09T07:18:43Z

src/validation/__tests__/KnownDirectivesRule-test.ts

@@ -219,9 +222,13 @@ describe('Validate: Known directives', () => {
        message: 'Directive "@onQuery" may not be used on FIELD.',
        locations: [{ column: 19, line: 16 }],
      },
+      {
+        message: 'Directive "@onField" may not be used on VARIABLE_DEFINITION.',


Yeah it's weird: if they're their own AST node then absolutely. If not... maybe? I think probably?

This is a small change and another thing I flip-flopped on.

mjmahone · 2023-02-09T07:21:09Z

src/validation/rules/NoUnusedVariablesRule.ts

      const usages = context.getRecursiveVariableUsages(operation);
      const variableNameUsed = new Set<string>(
-        usages.map(({ node }) => node.name.value),
+        usages
+          // Skip variables used as fragment arguments
+          .filter(({ fragmentVarDef }) => !fragmentVarDef)
+          .map(({ node }) => node.name.value),
      );


If we're OK making a breaking change. But Meta has custom validation rules that use getRecursiveVariableUsages so I imagine other people likely do too?

leebyron · 2023-02-09T17:55:59Z

Given our discussion at the APAC WG: if we're going to have a flag, I think it makes sense to split this PR into a series of dependent PRs.

@mjmahone I support this. I think we need exactly two:

Adds the Parse/Print/AST along with the experimental flag. There's an opportunity to get DirectiveLocations, and any other language-specific stuff right in this PR. Should be mostly uncontroversial since the language decisions seem agreed and the flag protects the ability to go back and correct if necessary.
Validation and Execution. Too hard to untangle these where behavior of one needs to influence the other, and iterating here is going to meaningfully impact spec text.

yaacovCR · 2024-09-26T20:44:51Z

Closing in favor of #4015 and maybe #4159

mjmahone mentioned this pull request Jan 19, 2023

Fragment Arguments: Spec Implementation graphql/graphql-spec#1010

Closed

mjmahone changed the title ~~Fragment Arguments Implementation~~ Fragment Arguments Reference Implementation Jan 19, 2023

mjmahone added spec RFC Implementation of a proposed change to the GraphQL specification PR: feature 🚀 requires increase of "minor" version number labels Jan 19, 2023

mjmahone mentioned this pull request Jan 19, 2023

Enable Fragment Arguments (a form of parameterized fragments) during execution #3152

Closed

This comment has been minimized.

Sign in to view

mjmahone commented Feb 9, 2023

View reviewed changes

src/language/parser.ts Outdated Show resolved Hide resolved

mjmahone force-pushed the fragment-args-2023 branch from 1fd2b9b to 02db413 Compare February 9, 2023 00:57

leebyron reviewed Feb 9, 2023

View reviewed changes

mjmahone commented Feb 9, 2023

View reviewed changes

mjmahone mentioned this pull request Feb 9, 2023

Fragment Arguments: Only Syntax Changes #3847

Closed

mjmahone changed the base branch from main to fragment-args-syntax February 9, 2023 21:53

mjmahone force-pushed the fragment-args-2023 branch from 650a99f to 59c89ef Compare February 9, 2023 22:07

mjmahone force-pushed the fragment-args-syntax branch from c845d38 to 2033b21 Compare February 9, 2023 22:10

mjmahone and others added 3 commits February 9, 2023 17:14

Fragment Arguments: Only Syntax Changes

4b1ae03

RFC: Fragment Arguments

e9fbda2

Add flag for parser

bfd80a6

mjmahone force-pushed the fragment-args-2023 branch from 59c89ef to 05c5ee3 Compare February 9, 2023 22:15

Split PRs, no functional changes

7fe0733

mjmahone force-pushed the fragment-args-2023 branch from 05c5ee3 to 7fe0733 Compare February 9, 2023 22:20

This was referenced Jan 27, 2024

feat: executing documents containing fragment-arguments JoviDeCroock/graphql-js#1

Closed

feat: execution for fragment arguments syntax JoviDeCroock/graphql-js#2

Merged

JoviDeCroock added a commit to JoviDeCroock/graphql-spec that referenced this pull request Feb 7, 2024

address graphql/graphql-js#3835 (comment)

2b2b071

JoviDeCroock mentioned this pull request Feb 7, 2024

Fragment args 2024 amendments mjmahone/graphql-spec#3

Closed

JoviDeCroock mentioned this pull request Feb 16, 2024

Fragment arguments/variables (syntax/validation/execution) graphql/graphql-spec#1081

Open

JoviDeCroock added a commit to JoviDeCroock/graphql-spec that referenced this pull request Mar 8, 2024

address graphql/graphql-js#3835 (comment)

76011b0

JoviDeCroock added a commit to JoviDeCroock/graphql-spec that referenced this pull request May 20, 2024

address graphql/graphql-js#3835 (comment)

6e91f98

yaacovCR closed this Sep 26, 2024

Fragment Arguments Reference Implementation #3835

Fragment Arguments Reference Implementation #3835

Conversation

mjmahone commented Jan 19, 2023 • edited Loading

netlify bot commented Jan 19, 2023 • edited Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

github-actions bot commented Jan 19, 2023

This comment has been minimized.

github-actions bot commented Feb 8, 2023

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron Feb 9, 2023 • 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

leebyron Feb 9, 2023 • 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

leebyron Feb 9, 2023 • 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

mjmahone 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

leebyron commented Feb 9, 2023

yaacovCR commented Sep 26, 2024

mjmahone commented Jan 19, 2023 •

edited

Loading

netlify bot commented Jan 19, 2023 •

edited

Loading

leebyron Feb 9, 2023 •

edited

Loading

leebyron Feb 9, 2023 •

edited

Loading

leebyron Feb 9, 2023 •

edited

Loading