Skip to content

[Refactor] Extract a shared init Feature interface for all product #8535

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

Merged
merged 10 commits into from
May 12, 2025

Conversation

fredzqm
Copy link
Contributor

@fredzqm fredzqm commented May 10, 2025

Previous: (setup: any, config: any, options?: any) => Promise<unknown>
Now: (setup: Setup, config: Config, options?: any) => Promise<unknown>

Mostly refactor to make TS compiler happy:

  • database: rewrite to use Setup type.
  • genkit: seems to be doing something bad or wrong with the Setup argument. Duct-tap it in the caller site for now.

Hoping to incrementally make it easier to define a shared askQuestion, then actuate split for products. The doSetup func can remain as the old way during the migration.

@fredzqm fredzqm enabled auto-merge (squash) May 12, 2025 18:34
@@ -132,7 +132,7 @@ export async function initGitHub(setup: Setup): Promise<void> {

// If the developer is using predeploy scripts in firebase.json,
// remind them before they set up a script in their workflow file.
if (setup.config.hosting.predeploy) {
if ((setup.config.hosting as any).predeploy) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the type system let us switch this to something like:

if (!Array.isArray(setup.config.hosting) && setup.config.hosting.predeploy) {
}

I think the existing code here is a bit broken and will never trigger if you have a HostingMulitple in your config. If you're feeling inspired, we could fix that, but since its just an informational mssage I'm ok leaving it as is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested, it does accept that.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 51.01%. Comparing base (702c1c0) to head (1b47936).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/extensions/manifest.ts 0.00% 2 Missing ⚠️
src/init/features/hosting/github.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8535      +/-   ##
==========================================
- Coverage   51.04%   51.01%   -0.03%     
==========================================
  Files         426      426              
  Lines       30562    30598      +36     
  Branches     6271     6276       +5     
==========================================
+ Hits        15599    15609      +10     
- Misses      13574    13601      +27     
+ Partials     1389     1388       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fredzqm fredzqm merged commit f322cb9 into master May 12, 2025
49 of 50 checks passed
@github-project-automation github-project-automation bot moved this from Approved [PR] to Done in [Cloud] Extensions + Functions May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants