update readme #272
update readme #272
Conversation
Yuangwang
requested review from
a team,
jamesdaniels,
leoortizz and
tonyjhuang
as code owners
There was a problem hiding this comment.
mostly lgtm. but would wait on @egilmorez for word smitthing.
README.md
Outdated
| | EnvVarConfig.value | string |Value associated with the environment variable | | ||
| | EnvVarConfig.availability | RUNTIME | Where the variable will be available, for now this will always be RUNTIME | | ||
|
|
||
| Many of these fields are shared with apphosting.yaml as well so see [our documentation](https://firebase.google.com/docs/reference/apphosting/rest/v1beta/projects.locations.backends.builds#runconfig) for additional context. |
There was a problem hiding this comment.
I'd suggest:
"Many of these fields are shared with apphosting.yaml. See the runConfig reference documentation for additional context."
There was a problem hiding this comment.
Am I seeing an outdated diff? It looks like you did not include the suggested change (as well as various other nits)
|
|
||
| ## Overview | ||
|
|
||
| Firebase Hosting integrates with popular modern web frameworks including Angular and Next.js. Using Firebase Hosting and |
There was a problem hiding this comment.
Should this entire overview be retooled to refer to App Hosting instead of Firebase Hosting?
iiuc, we're going to keep "supporting" framework-aware CLI via the very similar docs on Devsite, while we basically transform this particular readme and repo to pertain to FAH adapters. It will be key to distinguish the two arenas I think.
There was a problem hiding this comment.
This is just a copy and past of the old docs to the subsection that they apply to. I talked to James earlier and we decided we can just keep the old docs here for now too since they still make sense for this section. I don't think either of us felt very strongly about this though, so if you think it makes sense to remove this section as a whole I'm good with that too
Co-authored-by: Daniel Lee <[email protected]>
Went through and addressed/responded to all comments, would love another pass thanks! |
README.md
Outdated
| Firebase provides this functionality through the Firebase CLI. When initializing Hosting on the command line, you | ||
| provide information about your new or existing Web project, and the CLI sets up the right resources for your chosen Web | ||
| framework. | ||
| This repo holds the code for the adapters that enable support for these frameworks. At a high level these adapters transform frameowkr specific configurations into an output bundle spec that App Hosting is able to process and configure frameworks support. For more information see our [documentation](https://firebase.google.com/docs/app-hosting/about-app-hosting#frameworks) |
There was a problem hiding this comment.
I think this may have been an old diff, not sure what happened but these were already addressed for me locally and in the pr I saw. I just pushed another change to address a couple other changes too so could you check again and see if these are fixed now?
README.md
Outdated
| | EnvVarConfig.value | string |Value associated with the environment variable | | ||
| | EnvVarConfig.availability | RUNTIME | Where the variable will be available, for now this will always be RUNTIME | | ||
|
|
||
| Many of these fields are shared with apphosting.yaml as well so see [our documentation](https://firebase.google.com/docs/reference/apphosting/rest/v1beta/projects.locations.backends.builds#runconfig) for additional context. |
There was a problem hiding this comment.
Am I seeing an outdated diff? It looks like you did not include the suggested change (as well as various other nits)
I think they should have been pushed and addressed, the pr I saw pushed had these things addressed. Maybe some weird issue with github. Could you check again now? I just pushed some more changes to address the new comments but that should also include the old changes too. |
README.md
Outdated
| Firebase provides this functionality through the Firebase CLI. When initializing Hosting on the command line, you | ||
| provide information about your new or existing Web project, and the CLI sets up the right resources for your chosen Web | ||
| framework. | ||
| This repo holds the code for the adapters that enable support for these frameworks. At a high level these adapters transform framework specific configurations into an [output bundle spec](#app-hosting-output-bundle) that App Hosting can use to configure frameworks support. For more information see our [Framework integration](https://firebase.google.com/docs/app-hosting/about-app-hosting#frameworks). |
There was a problem hiding this comment.
Nit: omit "our" for just "see [Framework integration]"
|
Left some comments, I think we should consider having optional vs. required and defaults documented |
Added optional vs required and linked out to product docs for defaults documented so we have a single source of truth still. See discussion for another decision we've made before similar to this #244 (comment) |
Update the readme with output bundle specifications information