Skip to content
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

Initialize App Check debug mode in initializeAppCheck #5512

Merged
merged 6 commits into from
Sep 23, 2021
Merged

Conversation

hsubox76
Copy link
Contributor

Initialize debug mode for App Check (i.e., reading the global var) in the first call to initializeAppCheck() instead of when the app-check component is registered. This makes it easier for developers to set the variable in code.

Additionally, it logs a console.warn on subsequent calls to initializeAppCheck notifying the user they are still in debug mode, and logging the token (if they don't reload/quit before that token promise resolves). This may help HMR users who lose the initial token message.

@hsubox76 hsubox76 requested a review from Feiyang1 as a code owner September 17, 2021 19:08
@changeset-bot
Copy link

changeset-bot bot commented Sep 17, 2021

⚠️ No Changeset found

Latest commit: 675fb02

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2021

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@google-oss-bot google-oss-bot added the doc-changes PRs that affect docs label Sep 17, 2021
packages/app-check/src/api.ts Outdated Show resolved Hide resolved
packages/app-check/src/api.ts Outdated Show resolved Hide resolved
@Feiyang1 Feiyang1 assigned hsubox76 and unassigned egilmorez Sep 17, 2021
@hsubox76 hsubox76 assigned Feiyang1 and unassigned hsubox76 Sep 21, 2021
@@ -67,14 +66,13 @@ export function initializeAppCheck(
// Log a warning when `initializeAppCheck()` is called in debug mode,
Copy link
Member

Choose a reason for hiding this comment

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

Comment needs update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

expect(consoleStub.args[0][0]).to.include(token);
self.FIREBASE_APPCHECK_DEBUG_TOKEN = undefined;
});
it('warns about debug mode on second call', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

What does it test?
I think we need a test to verify initializeDebugMode is called only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, the test caught the fact that I was not setting the initialize property... thanks.

@Feiyang1 Feiyang1 assigned hsubox76 and unassigned Feiyang1 Sep 21, 2021
@hsubox76 hsubox76 assigned Feiyang1 and unassigned hsubox76 Sep 22, 2021
@hsubox76 hsubox76 merged commit 785a217 into master Sep 23, 2021
@hsubox76 hsubox76 deleted the ch-ac-debug branch September 23, 2021 18:47
@Feiyang1
Copy link
Member

Please add a changeset. @hsubox76

hsubox76 added a commit that referenced this pull request Sep 24, 2021
Feiyang1 pushed a commit that referenced this pull request Sep 24, 2021
@firebase firebase locked and limited conversation to collaborators Oct 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
doc-changes PRs that affect docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants