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

Swappable operation implementations #115

Merged
merged 17 commits into from
Jan 3, 2024
Merged

Swappable operation implementations #115

merged 17 commits into from
Jan 3, 2024

Conversation

pmelab
Copy link
Contributor

@pmelab pmelab commented Nov 20, 2023

Description of changes

  • Introduce a new system for swapping execution of GraphQL operations. The new shared package @amazeelabs/executors allows to register execution functions for specific GraphQL operations and parameter combinations.
  • Unify all data-access to use useOperation and this new system under the hood.

Note: The package is currently part of the PR, but will be moved to silverback-mono before merging.

Motivation and context

This approach solves two problems:

  1. Take out the guesswork "where" data comes from in UI components. Right now, it depends on the implementation. Gatsby data comes from props, dynamic queries from useOperation. This binds the UI to the implementation and makes it harder to use components in other environments (e.g. previews). Now every data operation has a GraphQL operation in the schema package and the UI component always uses useOperation and the implementation (Gatsby, Decap, Next ...) controls how the query is resolved.
  2. Make it easier to swap out data sources for different purposes, even based on parameter matching (e.g. pre-render page 1 of content hub, resolve others with http-request to Drupal).

Use cases:

  • preview handling (drupal, decap ...)
  • simplify data-mocking in storybook
  • pre-rendering of listings
  • injection of global data (menus, breadcrumbs) without prop-drilling

How has this been tested?

  • Manually
  • Unit tests
  • Integration tests

@pmelab pmelab changed the title WIP Swappable operation implementations Nov 20, 2023
@pmelab pmelab force-pushed the operators branch 3 times, most recently from f8a626d to 9f881ba Compare December 8, 2023 13:23
@pmelab pmelab force-pushed the operators branch 2 times, most recently from 4c84529 to 52cb799 Compare December 27, 2023 20:38
@pmelab pmelab marked this pull request as ready for review December 28, 2023 15:33
@pmelab pmelab requested a review from Leksat December 28, 2023 15:33
Copy link
Member

@Leksat Leksat left a comment

Choose a reason for hiding this comment

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

Looks great in general!

"author": "Amazee Labs <[email protected]>",
"license": "ISC",
"dependencies": {
"@types/lodash-es": "^4.17.12",
Copy link
Member

Choose a reason for hiding this comment

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

devDependencies?

export function registerExecutor(
id: string,
variables: VariablesMatcher,
executor: Executor,
Copy link
Member

Choose a reason for hiding this comment

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

TS function overloading does not always play well. For example, guess the inferred types:

type FirstParam = Parameters<typeof registerExecutor>[0];
type LastParam = Parameters<typeof registerExecutor>[2];

Answer

I would suggest using a single object parameter:

export function registerExecutor(args: RegistryEntry) {
  registry.push(args);
}

IMHO it would make make the function calls easier to read:

registerExecutor({ executor: myFunc, id: 'foo-bar' });

Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would produce a more natural API:

// register an executor that matches everything
registerExecutor(iDoEverythingOverHttp);
// register one for a specific operation;
registerExecutor(MyOperationId, myOperationImplementation);
// for a specific set of input:
registerExecutor(ContentHubQuery, {page: 1}, staticFirstPage);

Compared to:

// register an executor that matches everything
registerExecutor({executor: iDoEverythingOverHttp});
// register one for a specific operation;
registerExecutor({id: MyOperationId, executor: myOperationImplementation});
// for a specific set of input:
registerExecutor({id:ContentHubQuery, args: {page: 1}, executor: staticFirstPage});

I'm not sure if it has to be possible to infer types here, since theOperation* type helpers are available to infer return types and variables:

type first = AnyOperationId;
type second = Partial<OperationVariables<first>>;
type third = () => Promise<OperationResult<first>>;

But I also don't have a very strong opinion on this. How strong are yours? Should we pull in more voices?

* @param variables
* A dictionary of variables to be passed to the operation.
*/
export function createExecutor(id: string, variables?: Record<string, any>) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the return type?

Copy link
Member

Choose a reason for hiding this comment

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

BTW, in my pet projects I use '@typescript-eslint/explicit-function-return-type': 'error' eslint rule. But not sure if it will play well at work 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a great discussion point. Explicit means a type error is found earlier, not potentially through three levels of generic functions that infer something.

@pmelab pmelab merged commit 880dae2 into release Jan 3, 2024
5 checks passed
@pmelab pmelab deleted the operators branch January 3, 2024 19:34
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.

2 participants