refactor: clean up type casting in region resolution - #10504
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces EventTriggerResolutionTarget and BlockingTriggerResolutionTarget interfaces to replace the use of any and complex intersection types during region resolution. Feedback identifies that BlockingTriggerResolutionTarget is missing a project property and notes an unsafe type cast to backend.Endpoint which circumvents TypeScript safety checks.
Sorry, something went wrong.
| endpoint: backend.Endpoint & backend.BlockingTriggered, | ||
| ): string { | ||
| function resolveRegionForBlockingTrigger(endpoint: TriggerResolutionTarget): string { | ||
| if (!endpoint.blockingTrigger) { |
There was a problem hiding this comment.
I don't love this. Like we should only be calling this for blocking triggers and this was enforced by the old code right with a type of backend.Endpoint & backed.blockingTriggered
This seems to open us up to subtle bugs in the future where we call the wrong region resolution method and then incorrectly fall back to the default function region.
Sorry, something went wrong.
There was a problem hiding this comment.
The root problem seems to be that we're trying to construct a single endpoint ish object that has everything the method needs. But we don't really have to do that right? I realize the overall thing is working w/ endpoints and backends. But we don't have to only pass those around.
What if these methods just took individual args for only the info the needed.
E.g. this one only needs the blockingTrigger which already has a type and the resolveRegionForEventTrigger only needs eventTrigger and project?
I guess the issue with that is nominally we need a single interface if we want to in our next refactoring move these methods onto the service so we just call
service.resolveRegionForEventTrigger
But we could just also provide project for blocking triggers even though it's not used?
Thoughts?
Sorry, something went wrong.
There was a problem hiding this comment.
Switching to individual args makes the code much cleaner.
I think when we eventually refactor this to be part of the service, we can continue to use backend.Endpoint as the arg for a resolveRegion function, and extract the information from the endpoint we need per service, like the rest of the API calls in the Service interface:
Sorry, something went wrong.
ajperel
left a comment
There was a problem hiding this comment.
Glad the suggestions were useful. Thanks for cleaning this up :)
Sorry, something went wrong.
* refactor: clean up type casting in region resolution * Consolidate interface and refactor AILogic checks * Replace interface for individual args * Cleanup ailogic file
Description
This PR refactors the region resolution logic in src/deploy/functions/prepare.ts to eliminate unneeded object spreading ({ ...endpoint, id } as any) and loose type casting. Instead of forcing regional helper functions to accept a monolithic backend.Endpoint union type, the helpers have been updated to accept only the specific properties or trigger configurations they actually require.
Changes
src/deploy/functions/services/ailogic.ts:
src/deploy/functions/prepare.ts: