Skip to content

fix(hosting): name the function and regions in the wrong-region rewrite error - #11297

Draft
IzaakGough wants to merge 1 commit into
mainfrom
@invertase/fix-issue-11289
Draft

IzaakGough wants to merge 1 commit into
mainfrom
@invertase/fix-issue-11289

Conversation

@IzaakGough

Copy link
Copy Markdown
Contributor

Fixes #11289.

When a Hosting rewrite pointed at a function in the wrong region, the deploy failed with an error that did not say which function or which regions were involved.

The error now names the function, the region in the rewrite, and the regions where the function was found.

  • The error is thrown inside findEndpointForRewrite, next to the similar multi-region error, so the caller no longer needs its own wrong-region check.

Tested with unit tests and a live deploy --only hosting reproducing the issue. A correct-region deploy was not rerun live.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request improves the error messages shown when a Firebase Hosting rewrite targets a function in the wrong region. It updates findEndpointForRewrite to throw a detailed FirebaseError specifying the target region and listing the regions where the function was actually found, and updates the corresponding unit tests. The reviewer suggested a minor improvement to make the error message even more precise by outputting the actual region name instead of the generic phrase "that region" when only one region is available.

Comment on lines +84 to +88
throw new FirebaseError(
`Rewrite for function \`${id}\` in firebase.json targets region \`${region}\`, ` +
`but \`${id}\` was only found in ${regions}. Update the rewrite's \`region\` ` +
`in firebase.json to ${endpoints.length === 1 ? "that region" : "one of these regions"}.`,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of using the generic phrase "that region" when there is only one matching region, we can make the error message even more precise and actionable by outputting the actual region name (which is already formatted in the regions variable). This provides the user with a direct, copy-pasteable value for their firebase.json configuration.

Note: If you apply this suggestion, you will also need to update the corresponding unit tests in src/deploy/hosting/convertConfig.spec.ts to match the updated error message.

    throw new FirebaseError(
      `Rewrite for function \\\\`${id}\\\\\` in firebase.json targets region \\\\`${region}\\\\\`, ` +
        `but \\\\`${id}\\\\\` was only found in ${regions}. Update the rewrite's \\\\`region\\\\\` ` +
        `in firebase.json to ${endpoints.length === 1 ? regions : "one of these regions"}.`,
    );
References
  1. Error messages should clearly explain what failed and why; when actionable, provide concrete remediation guidance. (link)

This branch has not been deployed

No deployments
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.

Hosting deploy with Function rewrite failed with cryptic message

1 participant