Edit configs at the AST level to preserve formatting and comments #6987
Conversation
|
|
||
| import * as fs from "../fsutils"; | ||
| import { NodeType } from "yaml/dist/nodes/Node"; |
There was a problem hiding this comment.
This seems like an internal type for the library, and they explicitly don't use it in their public API. https://github.com/eemeli/yaml/blob/main/src/public-api.ts
Do you need this type? Could this be refactored not to use it?
There was a problem hiding this comment.
I think this is an oversight for it not to be exported. AFAICT none of the types are exported (note in public-api, Node is imported, and is a mandatory base class of Document<T, Strict> but is not exported in that file either.
Frankly, the type annotations are a mess; I had it working and removed it all. I could hypothetically remove the API call, but it would instead involve yet more casting as any/unknown.
* Fix --only firestore:rules,firestore:indexes edge case * Added changelog entry
…tions (#6990) * fix the falsy value from not being calculated * add changelog entry
| @@ -55,12 +56,45 @@ export function yamlPath(cwd: string): string | null { | |||
| } | |||
|
|
|||
| /** Load apphosting.yaml */ | |||
| export function load(yamlPath: string): Config { | |||
| export function load(yamlPath: string): yaml.Document { | |||
There was a problem hiding this comment.
optional nit but these methods all feel like good candidates to wrap in a yaml.ts that can be shared across firebase products.
Especially the main logic in getEnv and setEnv could likely be genericized and get/setEnv can use those helpers.
There was a problem hiding this comment.
Improves FAH secrets set command with AST-based YAML editing.
Replaces js-yaml in our codebase with yaml for consistency (though nowhere else needs the AST API). Unfortunately, js-yaml is still a dependency for our tooling, so it's still in package-lock.json
Want review from Tony for FAH stuff and Joe for CLI dependency changes + double checking extensions changes