Implement Unregister

[zwu52]

• unregister is backward compat. Calling it only deletes fid if applicable. If token exist is in local db. this method doesn't touch it.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d5b5eeb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[gemini-code-assist]

Code Review

This pull request introduces the unregister functionality to the Firebase Messaging package, allowing app instances to delete their FID-based registrations from both the server and local IndexedDB storage. The implementation includes new API exports, internal request handling, and comprehensive unit tests. Feedback from the review highlights the need to serialize the unregister operation using _registerNotifyChain to prevent race conditions with concurrent registration calls. Additionally, it is recommended to treat HTTP 404 responses as successful unregistrations and to simplify the error-handling logic in the network request layer.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/messaging/src/api/unregister.ts (33-68)

The unregister operation should be serialized using messaging._registerNotifyChain to prevent race conditions with concurrent register calls. This ensures that state updates (like lastNotifiedFid) and event notifications (onUnregistered) occur in the correct order relative to registration events. Additionally, the chain should be kept healthy by catching errors so that subsequent operations are not blocked.

export async function unregister(messaging: MessagingService): Promise<void> {
  if (!navigator) {
    throw ERROR_FACTORY.create(ErrorCode.AVAILABLE_IN_WINDOW);
  }

  const task = messaging._registerNotifyChain.then(async () => {
    // Prefer the last successfully registered FID from local metadata when available.
    const stored = await dbGetFidRegistration(
      messaging.firebaseDependencies
    ).catch(() => undefined);
    const fid =
      stored?.fid ??
      (await messaging.firebaseDependencies.installations.getId());

    await requestDeleteRegistration(messaging.firebaseDependencies, fid);

    // Best-effort local cleanup; still resolve even if schema is unavailable.
    try {
      await dbRemoveFidRegistration(messaging.firebaseDependencies);
    } catch {
      // Ignore.
    }

    if (messaging.lastNotifiedFid === fid) {
      messaging.lastNotifiedFid = null;
    }

    const handler = messaging.onUnregisteredHandler;
    if (handler) {
      if (typeof handler === 'function') {
        handler(fid);
      } else {
        handler.next(fid);
      }
    }
  });

  messaging._registerNotifyChain = task.catch(() => {});
  return task;
}

packages/messaging/src/internals/requests.ts (195-212)

For DELETE requests, a 404 Not Found response should be treated as a success, as it indicates the registration is already gone on the server. This allows the client to proceed with local cleanup. Also, the error handling can be simplified to avoid throwing strings and catching them immediately.

  if (response.ok || response.status === 404) {
    return;
  }

  // Best-effort parse error details; surface uniform error code.
  let errorInfo: string;
  try {
    const responseData = (await response.json()) as ApiResponse;
    errorInfo = responseData.error?.message ?? response.statusText;
  } catch (err) {
    errorInfo = response.statusText || (err as Error)?.toString();
  }

  throw ERROR_FACTORY.create(ErrorCode.FID_UNREGISTER_FAILED, { errorInfo });

[Doris-Ge]

Can we add two unit tests to verify the changes?

1. when register() -> deleteToken() -> register() happens, the second register() will still make a call to backend.
2. when getToken() -> unregister() -> getToken() happens, the second getToken() will make a call to backend.

I assume the changes to invalidate the token/fid cache during register()/getToken() will be included in a separate PR, is that correct?

[Doris-Ge]

After this PR is submitted, could you manually verify that a call to unregister() when the app is not registered will still succeed? I'd like to confirm that a delete request to our backend does not error out in this case.

[zwu52]

Can we add two unit tests to verify the changes?

1. when register() -> deleteToken() -> register() happens, the second register() will still make a call to backend.
2. when getToken() -> unregister() -> getToken() happens, the second getToken() will make a call to backend.

I assume the changes to invalidate the token/fid cache during register()/getToken() will be included in a separate PR, is that correct?

• added tests
• yes. following this one

[zwu52]

After this PR is submitted, could you manually verify that a call to unregister() when the app is not registered will still succeed? I'd like to confirm that a delete request to our backend does not error out in this case.

noted.

[zwu52]

ops. merged before commit & sync the fixes. creating a follow up pr