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

Overhaul Core #1236

Open
1 of 21 tasks
jasonkuhrt opened this issue Oct 28, 2024 · 0 comments
Open
1 of 21 tasks

Overhaul Core #1236

jasonkuhrt opened this issue Oct 28, 2024 · 0 comments

Comments

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 28, 2024

Perceived Problem

  • Various mostly internal but also extension-affecting unresolved design issues with core, hook inputs, what belongs in the require pipeline etc.

  • Reduce base bundle size #1216 that requires making transport modular (get memory out by default)

Ideas / Proposed Solution(s)

Request Core

  • In general request core needs to look up transport in registry based on selected transport type.
  • Transport logic takes place across three hooks in the request pipeline:
    - pack
    - exchange
    - unpack
  • A transport must supply all three hooks.
  • A transport receives a GraphQL Request (to pack) and returns a GraphQL Result (from unpack).
  • A transport may expose slots

Transport Extensibility

const httpTransport = createExtension({
	name: 'HttpTransport',
	transport: {
	  name: 'http',
		pack: {
			run: ({ graphqlRequest }) => {
				// ...
				return graphqlRequest
			},
			slots: {
				searchParams: getRequestEncodeSearchParameters,
				body: postRequestEncodeBody,
			},
		},

		exchange: {
			run: ({ slots, input }) => { // request specialized for this transport
				return slots.fetch(input.request)
			},
			slots: {
				fetch: (request) => fetch(request),
			}
		},

		unpack: {
			run: ({ graphqlResult }) => {
				return graphqlResult
			}
		}
	}
})

Extension Authors

  • Way to declare other extension dependencies.
  • In this example Upload needs MemoryTransport.
    - context.transportType becomes typed
    - pack slot $body becomes typed
export const Upload = () =>
  createExtension<{ needs: [HttpTransport] }>({
    name: `Upload`,
    onRequest: async ({ pack }, context) => {
      // TODO we can probably get file upload working for in-memory schemas too :)
      if (context.transportType !== `http`) {
        throw new Error(`Must be using http transport to use "Upload" scalar.`)
      }

      // Remove the content-type header so that fetch sets it automatically upon seeing the body is a FormData instance.
      // @see https://muffinman.io/blog/uploading-files-using-fetch-multipart-form-data/
      // @see https://stackoverflow.com/questions/3508338/what-is-the-boundary-in-multipart-form-data
      // @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition
      return await pack({
				input: {
          ...pack.input,
          headers: {
						...pack.input.headers,
            'content-type': '',
          },
        },
				$body: ({ variables, query }) => {
					const hasUploadScalarVariable = variables && isUsingUploadScalar(variables)
					if (!hasUploadScalarVariable) return

					// TODO we can probably get file upload working for in-memory schemas too :)
					if (context.transportType !== `http`) {
						throw new Error(`Must be using http transport to use "Upload" scalar.`)
					}

					return createBody({
						query,
						variables,
					})
				}
      })
    },
  }) 

User Experiences

Prepared

import { Graffle } from 'graffle'

Graffle
	.create()
	.transport({
		url: '...',
		// ...
	})

We must block access to request methods before a transport's minimum configuration has been set. This requirement challenges the pre-filled client. Normally we would have required a url in the http transport extension, but pre-filled wants to apply that extension before it gets to the user. In that sense pre-filled wants to move required extension configuration to the pre-filled constructor... maybe there is a way to reify this idea into the system, it not be ad-hoc or idea only.

const create = Graffle.createPreFilled().use(Extension1).use(Extension2)

The rule would be that each extensions constructor configuration gets added to the derived constructor parameters under a camel-case property name matching the extension name.

const graffle = create({
  extension1: { ... },
  extension2: { ... },
})

I can see a desire for parameter mapping because e.g. we would want HttpTransport to be configurable under transport. Furthermore we could allow a group key in case there are conflicts for the group name. So for example:

const create = Graffle.createPreFilled().use(HttpTransport)
const graffle = create({
  transport: { ... },
})
const create = Graffle.createPreFilled().use(HttpTransport).use(MemoryTransport)
const graffle = create({
  transport: {
    http: {...},
    memory: {...},
  },
})

So overall new requirements:

  1. Extensions can declare a group name
  2. Extensions can declare a group member name (used when multiple extensions ask for the same group name)
  3. A createPreFill utility that accepts extensions and other configuration and emits a constructor with that context pre-filled.
  4. Extensions need to have the concept of input and input normalization brought into the extension itself so that it can be leveraged by pre-filled in an automatic way. So probably we need a new extension field called normalizeInput or getConfig whose input and return types are statically brought into the overall extension type. It may be that it's prudent to expose new type variables on the extension interface to capture this or perhaps we end up being able to rely on just inference...

Memory Transport With Prepared

import { Graffle } from 'graffle'
import { MemoryTransport } from 'graffle/extensions/memory-transport'

Graffle
	.create()
	.use(MemoryTransport)
	.transport('memory')

Memory Transport With Raw

import { Graffle } from 'graffle/raw'
import { MemoryTransport } from 'graffle/extensions/memory-transport'

Graffle
	.create()
	.use(MemoryTransport)

Switch Between Transports

import { Graffle } from 'graffle'
import { MemoryTransport } from 'graffle/extensions/memory-transport'

Graffle
	.create()
	.use(MemoryTransport)
	.transport('memory')
    .transport('http')
    .transport('memory', { ... })
    // etc.

New Features Summary

  • New transport method to select a transport
  • When no transport is registered then transport method should be never or something like that.
  • When only one transport is registered then allow omitting the type.
  • When only one transport is registered then when nothing is specified default to using that one.
  • Transport becomes extensible
  • Extensions can receive a transport definition
  • Extensions can statically declare dependency on another extension
  • Slots are passed with dollar prefix at the same key level as input
  • request pipeline has no transport logic itself and instead just does a lookup of given transport type into transport registry and to execute that transport driver
  • "Transport Driver" term?
  • two new extensions HttpTransport and MemoryTransport
  • HttpTransport is pre-used
  • expose low level graffle constructor that has NO extensions pre-used
  • enforce extension dependencies statically, using one should check for presence of needs
  • remove concept of interface from require pipeline
  • internal refactor: restore custom scalars as a core request feature, encode/decode hooks
  • Without a transport the request pipeline must throw.
  • Extensions can declare a group name
  • Extensions can declare a group member name
  • createPreFill utility
  • Extensions need to have the concept of input and input normalization brought into the extension itself
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant