Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "implicit apply" behavior #355

Merged
merged 26 commits into from
Jun 7, 2023
Merged

Remove "implicit apply" behavior #355

merged 26 commits into from
Jun 7, 2023

Conversation

benjie
Copy link
Member

@benjie benjie commented May 30, 2023

Previously we would track the fieldArgs that you accessed (via .get(), .getRaw() or .apply()) and those that you did not access would automatically have their applyPlan called, if they had one. This isn't likely to be particularly useful for pure Grafast users (unless they want to adopt this pattern) but it's extremely useful for plugin-based schemas as it allows plugins to add arguments that can influence their field's plan without having to wrap the field's plan resolver. This is fairly critical, otherwise each behavior added (first:, condition:, orderBy:, filter:, ignoreArchived:, etc etc) would wrap the plan resolver another layer, and they would get messy.

However, implicit is rarely good. And it turns out that it severely limited what I wanted to do for improving the fieldArgs APIs - e.g. adding $ accessor shortcuts so instead of:

function add(_, fieldArgs) {
  const $a = fieldArgs.getRaw('a');
  const $b = fieldArgs.getRaw('b');
  return add($a, $b);
}

You could just do:

function add(_, { $a, $b }) {
  return add($a, $b);
}

After a (long!) chat with @ab-pm and a reasonable amount of experimentation, I landed on this solution.

TODO:

  • Rebase ontop of the HOTFIX
  • Extract some of these fixes/changes into separate PRs to minimize the size of this one
  • Remove the $ shortcuts, add them in a fresh PR that also improves the types
  • Review that the new names are the right names

@benjie benjie changed the base branch from planning to behavior-overhaul May 30, 2023 19:59
Base automatically changed from behavior-overhaul to planning June 7, 2023 11:31
@benjie benjie force-pushed the no-implicit-apply branch 2 times, most recently from 75f8860 to c4da7e7 Compare June 7, 2023 12:38
…rgs/input fields has been

removed.

Previously we would track the fieldArgs that you accessed (via `.get()`,
`.getRaw()` or `.apply()`) and those that you _did not access_ would
automatically have their `applyPlan` called, if they had one. This isn't likely
to be particularly useful for pure Gra*fast* users (unless they want to adopt
this pattern) but it's extremely useful for plugin-based schemas as it allows
plugins to add arguments that can influence their field's plan _without having
to wrap the field's plan resolver function_. This is fairly critical, otherwise
each behavior added (`first:`, `condition:`, `orderBy:`, `filter:`,
`ignoreArchived:`, etc etc) would wrap the plan resolver with another function
layer, and they would get _messy_.

However, implicit is rarely good. And it turns out that it severely limited
what I wanted to do for improving the `fieldArgs` APIs.

I decided to remove this implicit functionality by making it more explicit, so
now args/input fields can specify the relevant
`autoApplyAfterParent{Plan,SubscribePlan,InputPlan,ApplyPlan}: true` property
and we'll only apply them at a single level.

From a user perspective, little has changed. From a plugin author perspective,
if you were relying on the implicit `applyPlan` then you should now add the
relevant `autoApply*` property next to your `applyPlan` method.
@benjie benjie merged commit 5398d6b into planning Jun 7, 2023
@benjie benjie deleted the no-implicit-apply branch June 7, 2023 16:04
@benjie benjie added this to the V5 beta milestone Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant