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

Investigate contextual node trade-offs #1015

Open
ssalbdivad opened this issue Jun 15, 2024 · 1 comment
Open

Investigate contextual node trade-offs #1015

ssalbdivad opened this issue Jun 15, 2024 · 1 comment

Comments

@ssalbdivad
Copy link
Member

Currently, @arktype/schema nodes don't have properties like .parent or .path.

Adding them would have the following trade-offs, potentially among others:

Good:

  • contextualReferences/Disjoint wouldn't have to be built up/prefixed at each step with methods like withPrefixKey
  • Runtime traversal in cases requiring context (i.e. on invalid data, validators including morphs etc.) could be much faster

Bad:

  • Can't reuse cached instances of nodes like string that could be instantiated thousands of times at different paths
  • The fact that string at different paths etc. means we can also no longer trivially reuse intersection cache results for those nodes, although there may be a way to do that if we ensure the paths are only used in Disjoints? Requires investigation

Without further investigation, I'm not sure what the actual impact of these trade-offs would be, but it could be a significant improvement depending on the caching costs so worth looking into.

@github-project-automation github-project-automation bot moved this to To do in arktypeio Jun 15, 2024
@ssalbdivad ssalbdivad moved this from To do to Backlog in arktypeio Jun 16, 2024
@ssalbdivad
Copy link
Member Author

Did some initial investigation here and mostly got things working. Perf trade offs still need to be evaluated as using this prototype sharing method makes managing rebinding scopes more complex:

BaseNode.ts

export interface ContextualNodeProps {
	$: RawRootScope
}

export abstract class BaseNode<
	/** uses -ignore rather than -expect-error because this is not an error in .d.ts
	 * @ts-ignore allow instantiation assignment to the base type */
	out d extends RawNodeDeclaration = RawNodeDeclaration
> extends DynamicBase<
	((data: d["prerequisite"]) => unknown) & attachmentsOf<d>
> {
}
// etc

	readonly unbound = this
	bindContext(ctx: ContextualNodeProps) {
		const proto = Object.create(
			this.unbound,
			Object.getOwnPropertyDescriptors(ctx)
		)
		return Object.setPrototypeOf(this.unbound.traverse.bind(proto), proto)
	}

// etc
	traverse(data: d["prerequisite"], pipedFromCtx?: TraversalContext): unknown {
		// pipedFromCtx allows us internally to reuse TraversalContext
		// through pipes and keep track of piped paths. It is not exposed

		if (!this.includesMorph && !this.allowsRequiresContext && this.allows(data))
			return data

		if (pipedFromCtx) {
			this.traverseApply(data, pipedFromCtx)
			return pipedFromCtx.data
		}

		const ctx = new TraversalContext(data, this.$.resolvedConfig)
		this.traverseApply(data, ctx)
		return ctx.finalize()
	}

parse.ts

export const parseNode = <kind extends NodeKind>(
	kind: kind,
	schema: NormalizedSchema<kind>,
	$: RawRootScope,
	opts: NodeParseOptions
): BaseNode => {
	const prefix = opts.alias ?? kind
	nodeCountsByPrefix[prefix] ??= 0
	const id = `${prefix}${++nodeCountsByPrefix[prefix]!}`
	const ctx: NodeParseContext = {
		...opts,
		$,
		args: opts.args ?? {},
		schema,
		id
	}
	return _parseNode(kind, ctx).bindContext({ $: ctx.$ })
}

const _parseNode = (kind: NodeKind, ctx: NodeParseContext): BaseNode => {
// etc
}

This could allow us to store contextual props like scope, parent and path on the bound node without breaking caching for the unbound node. It also means the base nodes could avoid inheriting Callable and could instead extend DynamicBase, since when they are bound they become callable.

I was also considering again trying to remove scope altogether from nodes in @arktype/schema and have the opreations that parsed like intersection instead defined on the scope, but it sounded like scope creep so can revisit this later :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

1 participant