-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Getting autoinit working with the App Hosting emulator #8582
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
blidd-google
left a comment
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.
left some comments / nits but otherwise LGTM
| ): Promise<void> { | ||
| const npmLs = spawnSync("npm", ["ls", "@firebase/util", "--json", "--long"], { | ||
| cwd: rootDirectory, | ||
| shell: process.platform === "win32", |
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.
We only want to run npm ls in a shell if the user is on windows?
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 saw that pattern elsewhere in the CLI, seems to be working fine on mac/linux. Ideally though in an upcoming release I can ditch this in favor of cookie config.
| const apphostingEmulatorConfig = options.config.src.emulators?.[Emulators.APPHOSTING]; | ||
|
|
||
| if (listenForEmulator.apphosting) { | ||
| const rootDirectory = apphostingEmulatorConfig?.rootDirectory; |
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.
How does the root directory here relate to the root directory specified per-backend by the user in firebase.json?
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 think we need to put some thought into that, the main difference IMO is that emulatorConfig rootDir is relative to the json. I think my matcher works well enough for now but as firebase init apphosting is new I might be missing some cases. Will play around.
What's new
Pretty simple one, wire up
FIREBASE_CONFIG,FIREBASE_WEBAPP_CONFIG, and a few other important environment variables to the App Hosting emulator to support SDK autoinit—from there trip@firebase/utilspostinstall step, if present. This is intended to fail open and rely on the JS SDK error messages, if the developer is relying on autoinit.The only "meh" bit is cross referencing the apphosting emulator config with the new
.apphostingconfig entry, to look up thebackendId. We should consider aligning these more in the future.Cases tested
Known issues
rm -rf .next/cacheI will document these limitations. IMO the best solution to both of these is to use autoinit via Cookie—like Web Frameworks does—but that will take a bit more work as we'll need to implement a proxy, I will address post-I/O.
Discovered issues
While working on this feature I discovered a couple issues with the App Hosting emulator, namely that the child process isn't always cleanly terminated and never is when using
emulators:exec. I'll file bugs and if other folk don't have bandwidth I can work on that while addressing the above known issues