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

[RFC] Definition API -> Builder pattern? #63

Closed
Vorlias opened this issue Dec 15, 2021 · 8 comments
Closed

[RFC] Definition API -> Builder pattern? #63

Vorlias opened this issue Dec 15, 2021 · 8 comments
Labels
Enhancement New feature or request

Comments

@Vorlias
Copy link
Member

Vorlias commented Dec 15, 2021

I feel like the definitions API could be cleaner, perhaps instead of doing something like:

	[InventoryClientFunctionId.MoveEquippedItemToInventorySlot]: ServerAsyncFunction<
		(
			request: MoveEquippedItemToInventorySlotRequest,
		) => Serializer.NetworkResult<MoveEquippedItemToInventorySlotResponse, MoveEquippedItemToInventorySlotError>
	>([
		Net.Middleware.TypeChecking(
			t.interface({
				InventoryId: t.numberMin(0),
				SlotId: t.numberMin(0),
				EquipSlot: t.string,
			}),
		),
	]),

We could do

	[InventoryClientFunctionId.MoveEquippedItemToInventorySlot]: ServerAsyncFunction<
		(
			request: MoveEquippedItemToInventorySlotRequest,
		) => Serializer.NetworkResult<MoveEquippedItemToInventorySlotResponse, MoveEquippedItemToInventorySlotError>
	>().WithMiddleware([
		Net.Middleware.TypeChecking(
			t.interface({
				InventoryId: t.numberMin(0),
				SlotId: t.numberMin(0),
				EquipSlot: t.string,
			}),
		),
	]),

Could simplify this further for built-in middleware, such as .WithTypeChecking(...types) and .WithRateLimit(options)

	[InventoryClientFunctionId.MoveEquippedItemToInventorySlot]: ServerAsyncFunction<
		(
			request: MoveEquippedItemToInventorySlotRequest,
		) => Serializer.NetworkResult<MoveEquippedItemToInventorySlotResponse, MoveEquippedItemToInventorySlotError>
	>().WithTypeChecking(
		t.interface({
			InventoryId: t.numberMin(0),
			SlotId: t.numberMin(0),
			EquipSlot: t.string,
		}),
	),

This could play well with #62.

@Vorlias Vorlias added Enhancement New feature or request Unsure Further information is requested labels Dec 15, 2021
@Vorlias Vorlias changed the title [Draft] Definition API -> Builders? [Draft] Definition API -> Builder pattern? Dec 15, 2021
@OverHash
Copy link
Contributor

This looks a lot easier to read, doesn't require referring to documentation/IntelliSense to understand the order of parameters.

Would the .With* calls be chainable? (i.e., could you do .WithMiddleware([...]).WithTypeChecking([...])? How is the order of that resolved at call-time, from start to finish in the chain (i.e., the WithMiddleware is called first, and then the WithTypeChecking calls)?

@Vorlias
Copy link
Member Author

Vorlias commented Dec 15, 2021

This looks a lot easier to read, doesn't require referring to documentation/IntelliSense to understand the order of parameters.

Would the .With* calls be chainable? (i.e., could you do .WithMiddleware([...]).WithTypeChecking([...])? How is the order of that resolved at call-time, from start to finish in the chain (i.e., the WithMiddleware is called first, and then the WithTypeChecking calls)?

Yeah they'd be chainable. Order wouldn't necessarily matter as it'd just add to the middleware array - the calls would just modify the resulting object it uses anyway for the definition (like your typical builder)

@OverHash
Copy link
Contributor

Sounds good to me!

@Vorlias Vorlias changed the title [Draft] Definition API -> Builder pattern? [RFC] Definition API -> Builder pattern? Dec 15, 2021
@Vorlias Vorlias added RFC request for comments and removed Unsure Further information is requested labels Dec 15, 2021
@Vorlias
Copy link
Member Author

Vorlias commented Oct 18, 2023

Alrighty have a loose idea I'm playing around with, but being backwards-compat:

		TestV4ServerFunction: Net.Definitions.AsyncFunction()
			.EnsureArguments<[value: string]>(t.string)
			.EnsureReturns(t.string)
			.WithRateLimit({
				MaxRequestsPerMinute: 10,
			})
			.WithCallbackMiddleware((procNext, instance) => {
				return (player, name) => {
					if (someCondition) {
						return procNext(player, name);
					} else {
						return NetMiddleware.Skip;
					}
				};
			})
			.ForServer(),

Essentially will have ForServer and ForClient being the context-specific "build" functions. Note also NetMiddleware.Skip for skipping middleware processing further.

I ideally do want to make type checks mandatory but it's less backwards compatible and awkward with the async function.

@Vorlias Vorlias pinned this issue Oct 25, 2023
@Vorlias
Copy link
Member Author

Vorlias commented Oct 25, 2023

Net.Definitions.Create() without the arguments will use a builder:

const BuilderRemotes = Net.Definitions.Create()
	// Create a namespacae using an object model
	.AddNamespace("ExampleNamespaceObject", {
		ExampleEvent: Net.Definitions.Event().EnsureArguments(t.string).OnServer(), // Event for server taht takes
	})
	// Create a namespace using another builder + object model
	.AddNamespace(
		"ExampleNamespaceUsingInnerBuilder",
		Net.Definitions.Create().Add({
			// Example object-like
			ExampleAsyncFunction: Net.Definitions.AsyncFunction().OnClient(),
		}),
	)
	// Create a namespace with a builder functionally
	.AddNamespace(
		"ExampleNamespaceUsingFunctionalBuilder",
		builder => {
			// You can add remotes one by one functionally like this
			return builder
				.AddClientRemote("ClientRemoteName", Net.Definitions.AsyncFunction())
				.AddServerRemote("ServerRemoteName", Net.Definitions.Event());
		}, // add a client remote using a function
	)
	// Of course like our inner builder, we can use 'Add' here.
	.Add({
		ExampleBaseRemote: Net.Definitions.AsyncFunction().OnServer(),
		// Can add a created definition as a namespace to an object model
		ToNamespaceBehaviour: Net.Definitions.Create().ToNamespace(),
	})
	.AddServerRemote("NamedBaseServerRemoteFunction", Net.Definitions.AsyncFunction())
	.AddClientRemote("NamedBaseClientRemoteEvent", Net.Definitions.Event())
	.Build();

I'm also considering AddServerOwned/AddClientOwned instead of or in addition to Add - where it will handle the internal OnServer/OnClient for you.

@Vorlias Vorlias closed this as completed Oct 25, 2023
@Vorlias Vorlias reopened this Oct 25, 2023
@Vorlias
Copy link
Member Author

Vorlias commented Nov 4, 2023

image
👀

It's a bit more explicit than the following:-
image

However this should ensure types are checked etc.

@Vorlias
Copy link
Member Author

Vorlias commented Nov 4, 2023

image
Simplified a bit :-)

@Vorlias
Copy link
Member Author

Vorlias commented Nov 4, 2023

image
image

image

Lol 😆

@Vorlias Vorlias removed the RFC request for comments label Dec 4, 2023
@Vorlias Vorlias closed this as completed Dec 4, 2023
@Vorlias Vorlias mentioned this issue Dec 4, 2023
3 tasks
@Vorlias Vorlias unpinned this issue Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants