-
Notifications
You must be signed in to change notification settings - Fork 1k
Code to create emulators.yaml uses backend root not project root #8412
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
Conversation
NOTE: This PR is part of my chain for debugging cursor glitches, but if approved, I'll probably rebase onto next directly, commit there, and then re-pull it into inlined.inquirer. |
d46c95f
to
f1292ad
Compare
): Promise<Env[] | null> { | ||
// Even if the app is in /project/app, the user might have their apphosting.yaml file in /project/apphosting.yaml. | ||
// Walk up the tree to see if we find other local files so that we can put apphosting.emulator.yaml in the right place. | ||
const basePath = dynamicDispatch.discoverBackendRoot(repoRoot) || repoRoot; | ||
const basePath = dynamicDispatch.discoverBackendRoot(backendRoot) || backendRoot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const basePath = dynamicDispatch.discoverBackendRoot(backendRoot) || backendRoot; | |
const basePath = dynamicDispatch.discoverBackendRoot(backendRoot) ?? backendRoot; |
Nit: IIUC, ?? is preferred over || for undefined fallbacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer a style where you only use ??
when you know null
and/or 0
is a valid value.
Co-authored-by: joehan <[email protected]>
6dbdd66
to
b07c42c
Compare
* Use backend root, not project root to initilaize apphosting.emulators.yaml * Changelog & lint * Update CHANGELOG.md Co-authored-by: joehan <[email protected]> --------- Co-authored-by: joehan <[email protected]>
Quicker fix than I realized.
We may want to re-add some of the previous behavior to detect multiple environments and determine which to use as the base config. I was more focused on making breaking changes before the major release, but that was interesting/useful, esp if people only have apphosting[.environment].yaml and not a generic apphosting.yaml.
That might require more though though as we may want to consolidate backendRoot/apphosting.mybackend.yaml and /apphosting.yaml for mult-backend setups.