refactor: move default region resolution to service interface#10586
refactor: move default region resolution to service interface#10586wandamora wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors trigger region resolution by delegating the default region lookup to individual services via a new getDefaultRegion method on the Service interface. It also consolidates multi-region mappings and the isUSRegion helper into src/gcp/location.ts. Feedback on the changes suggests avoiding the use of as any in serviceForEndpoint by using the in operator for type narrowing, and removing the unused endpoint parameter in AuthBlockingService.getDefaultRegion instead of disabling the ESLint rule.
| return "us-east1"; | ||
| async function resolveRegionForTrigger(endpoint: build.Endpoint): Promise<string> { | ||
| const service = serviceForEndpoint(endpoint); | ||
| if (service.getDefaultRegion) { |
There was a problem hiding this comment.
I will do a full review soon but taking a quick glance now.
Do you think we should make this a required method for the Service interface? Then we'd never forget it for new services. But I don't know if we want to hard code the us-central default in a million places. Something to mull over.
| validateTrigger: noop, | ||
| registerTrigger: noop, | ||
| unregisterTrigger: noop, | ||
| getDefaultRegion: () => Promise.resolve("us-east1"), |
There was a problem hiding this comment.
Should we use a constant for this region?
3 participants
Description
This PR refactors the trigger default region resolution logic during Cloud Functions deployment to make it less brittle, easier to maintain, and more scalable.
Previously, prepare.ts contained complex service-specific logic and switch/if blocks to inspect different event types (Firestore, Cloud Storage, Realtime Database, etc.) and look up their corresponding default regions. This was highly brittle: if a new service or event type is added, developers would have to manually update prepare.ts, which could be easy to miss.