Fix README typos, grammar #448

hunghvu · 2022-03-15T23:42:03Z

Currently, useFirebaseAdminDefaultCredential is not in the InitConfig interface, so this causes Object literal may only specify known properties in TypeScript projects. This PR adds that property to the InitConfig interface.

Address miscellaneous typos, grammar in either the documents or code samples.

Side question: Running a test on my environment results in carriage return error in Prettier (error Delete ␍ prettier/prettier). I temporarily added 'prettier/prettier': ['error', { endOfLine: 'auto' }], in .eslintrc.js to workaround. To avoid modifying testing configuration, I did commit that, but should it be included in the PR?

vercel · 2022-03-15T23:42:07Z

@hunghvu is attempting to deploy a commit to the Gladly Team on Vercel.

A member of the Team first needs to authorize it.

kmjennison

Hi, thanks for the fixes and edits! Please:

break out the type addition into its own PR
see feedback on edits to the README

kmjennison · 2022-03-18T17:42:34Z

index.d.ts

@@ -67,6 +67,7 @@ interface InitConfig {
  tokenChangedHandler?: (user: AuthUser) => void
  onLoginRequestError?: (error: unknown) => void
  onLogoutRequestError?: (error: unknown) => void
+  useFirebaseAdminDefaultCredential?: boolean


Please break this out into its own PR.

kmjennison · 2022-03-18T17:43:51Z

README.md

@@ -33,7 +33,7 @@ We treat the Firebase JS SDK as the source of truth for auth status. When the us

 Depending on your app's needs, other approaches might work better for you.

-**If your app only uses static pages** or doesn't need the Firebase user for SSR, use the Firebase JS SDK directly to load the user on the client side.
+**If your app only uses static pages** or doesn't need the Firebase user for SSR, use the Firebase JS SDK directly to load the user on the client-side.


Splitting hairs here, but we aim to use "client side" for the noun and "client-side" for the adjective. Please keep nouns as-is.
https://docs.microsoft.com/en-us/style-guide/a-z-word-list-term-collections/c/client-side

Same for "server-side" and "server side".

kmjennison · 2022-03-18T17:46:15Z

README.md

-| `whenUnauthedBeforeInit`   | The action to take if the user is _not_ authenticated but the Firebase client JS SDK has not yet initialized. One of: `AuthAction.RENDER`, `AuthAction.REDIRECT_TO_LOGIN`, `AuthAction.SHOW_LOADER`.                                            | `AuthAction.RENDER`      |
-| `whenUnauthedAfterInit`    | The action to take if the user is _not_ authenticated and the Firebase client JS SDK has already initialized. One of: `AuthAction.RENDER`, `AuthAction.REDIRECT_TO_LOGIN`.                                                                      | `AuthAction.RENDER`      |
+| `whenUnauthedBeforeInit`   | The action to take if the user is _not_ authenticated but the Firebase client JS SDK has not yet been initialized. One of: `AuthAction.RENDER`, `AuthAction.REDIRECT_TO_LOGIN`, `AuthAction.SHOW_LOADER`.                                            | `AuthAction.RENDER`      |
+| `whenUnauthedAfterInit`    | The action to take if the user is _not_ authenticated and the Firebase client JS SDK has already been initialized. One of: `AuthAction.RENDER`, `AuthAction.REDIRECT_TO_LOGIN`.                                                                      | `AuthAction.RENDER`      |


There's a distinction between the developer having initialized it vs. Firebase completing its initialization. We're trying to convey the latter, which I think is clearer without "been". Please revert.

Interesting, my initial interpretation was about the process of Firebase being initialized by next-firebase-auth, hence the word been. I will revert this.

kmjennison · 2022-03-18T17:47:52Z

README.md


 > ℹ️ Using Vercel? See [adding a private key to Vercel](#adding-a-private-key-to-Vercel) for guidance.

 #### useFirebaseAdminDefaultCredential

 `Boolean`

-When true, `firebase-admin` will use the Google Cloud application default credentials during [`initializeApp`](https://firebase.google.com/docs/admin/setup#initialize-sdk).
+When true, `firebase-admin` will implicitly find your hosting environment service account during `initializeApp`. This is applicable for both [Firebase](https://firebase.google.com/docs/admin/setup#initialize-sdk), and [Google Cloud Platform](https://cloud.google.com/docs/authentication/production), and more recommended than adding service account key to the code via file path or direct value.


Please change: "more recommended than adding" --> "recommended over adding"

kmjennison · 2022-03-18T17:48:27Z

README.md

@@ -626,15 +626,15 @@ An async function that resolves to a valid Firebase ID token string, or null if

 **clientInitialized** - `Boolean`

-Whether the Firebase JS SDK has initialized. If `true`, we are no longer using any user info from server-side props.
+Whether the Firebase JS SDK has been initialized. If `true`, we are no longer using any user info from server-side props.


Same as above about "been initialized".

kmjennison · 2022-03-18T17:49:16Z

README.md

@@ -908,7 +908,7 @@ export default getMockAuthUser

 Now, you can use and customize the mock behavior in your tests.

-If you're modifying higher-order functions, component being tested needs to be `require`d inside a `beforeEach` function or inside each test case. This is because mocking `next-firebase-auth` has to happen _before_ your component is imported, because the call to the `next-firebase-auth` function is part of the default export of your component (e.g., `export default withAuthUser()(MyComponent)`).
+If you're modifying higher-order functions, components being tested will be `required` inside a `beforeEach` function or each test case. This is because mocking `next-firebase-auth` has to happen _before_ your component is imported, because the call to the `next-firebase-auth` function is part of the default export of your component (e.g., `export default withAuthUser()(MyComponent)`).


Change: "will be" --> "need to be". This makes it clear the developer is responsible.

vercel · 2022-04-01T16:29:07Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/gladly-team/nfa-example/J3a7vw8re62du2Y23UKefRMUvAhp
✅ Preview: https://nfa-example-git-fork-hunghvu-main-gladly-team.vercel.app

codecov · 2022-04-01T16:30:51Z

Codecov Report

Merging #448 (40d06fc) into main (e6fe5dd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #448   +/-   ##
=======================================
  Coverage   99.28%   99.28%           
=======================================
  Files          26       26           
  Lines         556      556           
  Branches      190      190           
=======================================
  Hits          552      552           
  Misses          4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6fe5dd...40d06fc. Read the comment docs.

kmjennison · 2022-04-01T16:32:39Z

Interesting that you're seeing a Prettier error locally when linting passes here. Maybe you're running a different version of Prettier from a global install? Regardless, doesn't look like it's an issue for this PR. Thanks for the improvements!

kmjennison

LGTM

Co-authored-by: Hung Vu <hunghvu2017@gmail.com>

* Debug release action * 0.14.0-alpha.0 * Revert "0.14.0-alpha.0" This reverts commit 1e04c95. * Remove --dry-run from release action * Display the Firebase version on the demo app * Update README.md * Update issue templates * feat: forceRefresh on getIdToken * feat: adding description about forceRefresh * feat: adding argument type to getIdToken * Update createAuthUser.js * 0.13.4-alpha.0 * 0.13.4 * Upgrade some dependencies (#325) * Upgrade some deps * Upgrade caniuse * Upgrade Prettier * Downgrade eslint to satisfy peerdeps * Minor upgrades for Next and Firebase deps * Upgrade firebase-admin * Upgrade firebase-admin in demo * Minor upgrade demo deps * Upgrade NFA version in demo * Upgrade more dependencies * Upgrade more demo dependencies * Revert "Upgrade more demo dependencies" This reverts commit 7da3e58. * Revert "Minor upgrade demo deps" This reverts commit 50f928e. * Minor upgrade Firebase * Pin typescript version (typing error with 4.4.4) and minor upgrade other types * Minor bump a few demo deps * Upgrade more demo deps * Upgrade some deps * Remove unused Codecov dependency * Support Next 12 and Firebase Admin 10 (#328) * Use Next 12 * Allow latest versions of Next and firebase-admin * 0.13.5-alpha.0 * Update demo to use Next 12 (#330) * Use Next 12 * Allow latest versions of Next and firebase-admin * Upgrade Next to v12 * Update demo * Update README.md * Update README.md * Change example app cookies to use SameSite=lax (#354) * 0.13.5 * feat: add support for application default credentials (#348) * feat: fallback to applicationDefault credentials Co-Authored-By: Jesse Anderson <jeryanders@gmail.com> * chore: add test for firebaseAdminDefaultCredential * chore: update README with firebaseAdminDefaultCredential Co-Authored-By: Jesse Anderson <jeryanders@gmail.com> * chore: cleanup wording in README comments * fix: updates based on comments * chore: update error message in test Co-authored-by: Jesse Anderson <jeryanders@gmail.com> * Update README.md * Upgrade dependencies on v0.x (#356) * Upgrade most deps * Upgrade ESLint and Prettier and lint fix * Remove unneeded jsdom dep * Upgrade most example app deps * Upgrade example app lockfile deps * Upgrade lockfile deps * Fix peer dependency range syntax for firebase-admin (#358) * Handle additional token errors in verifyIdToken (#361) (#365) * fix: check for 'auth/argument-error' when verifying token * feat: upgrade firebase and firebase-admin * feat(#174): handle additional errors from `verifyIdToken` * test(#174): add tests coverage for new errors in `verifyIdToken` * feat: upgrade dependencies that have non breaking changes * feat: implement pr feedback * chore: upgrade dependencies * Rebuild lockfile * Include error if empty refreshToken * Add TODO Co-authored-by: Zino Hofmann <zino@hofmann.amsterdam> Co-authored-by: Zino Hofmann <zino@hofmann.amsterdam> * Remove thrown errors from token refresh & verification logic (#368) * Add broken tests * Don't throw on token errors * Add assertion checks to tests * Add/modify comments in config validation * Move tests into describe block * Add error callbacks to config * Call error callbacks when we fail to refresh or verify the user's ID token * Add new config properties to types * Await error callback functions in case they need to perform something async * Lint fixes * Add tests * Update README.md * Fix typo in README (#374) * Cherry pick v1.x #369 (#375) Co-authored-by: Faris Abusada <abusada@users.noreply.github.com> * Allow easy error handling for login/logout requests (#376) * Add error catching to default token changed handler * Add new config properties * Update README.md * Update README.md * Run Prettier on README (#381) * 0.14.0-alpha.o * Update README.md * 0.14.0-alpha.1 * Update v0.x example (#382) * Update documentation (#387) * Update docs on private key formatting and Vercel environment variables (#385) * Update README.md * Update README.md * Update README.md * Update README.md * Update README.md * Add note about not using API routes in getServerSideProps (#386) * 0.14.0 * Added troubleshooting step to README (#398) * Update README.md * feat: add ability to define to redirect to app with different base path (#352) * feat: add generalized redirect AuthAction * test: add tests for csr and ssr withAuthUser * fix: made error message more focused on a given auth state * fix: removed new AuthAction and address issue 187 with new solution * test: add main tests for new schema on appPageURL and authPageURL * refactor: move destination logic into common function * refactor: add redirects * refactor: error messaging around redirects * fix: router and window location * test: add test coverage for new supported data type * docs: rework docs * fix: error message for authPageURL * fix: bug in ssr component and slightly later url schema * docs: change schema property * fix: allow basePath on SSR to be passed based on findings * chore: make naming consistent between csr / ssr * fix: rework based on feedback * refactor: bring config access into redirect module * style: spacing and formatting * docs: update readme and types * refactor: simplify object name * fix: update typescript * tests: fix tests causing coverage issues * fix: rework from feedback * fix: implement feedback * test: add additional test * test: adjust test name * Update documentation for redirects (#400) * Add PageURL type to README * Typo fix * Link to PageURL type in docs * Run Prettier on README * Fix incorrect documentation args * Link to PageURL type from example * Tweak README * Fix typo * Add info about ctx * Remove redundant info * Tweak code comments * 0.14.1-alpha.0 * Update example app (#402) * 0.14.1 * added onLogoutRequestError and onLoginRequestError to InitConfig interface (#427) * feat: add tenant integration * Update link to documentation * 0.14.2 * Bugfix: don't error on unset Firebase admin config values (#436) * Identify bug * When debug logging, handle unset Firebase config values * Lint fix * 0.14.3-alpha.0 * fix: Add useFirebaseAdminDefaultCredential type definition (#451) * Fix README typos, grammar (#448) * Add useFirebaseAdminDefaultCredential type definition * Fix typos, grammar, and clarify Google default credentials usage * docs: Fix grammar, remove type addition from PR * 0.14.3-alpha.1 * 0.14.3 * Upgrade NFA version in demo (#455) * Upgrade NFA version in demo * Update min version * v0.x: Add support for React 18 (#472) * Add support for React 18 * Upgrade some testing libraries * v0.x: upgrade dependencies (#477) * Upgrade most deps * Upgrade additional deps * 0.14.4-alpha.0 * v0.x: Update example app (React 18, other dependencies) (#471) * Update example to use React 18 * Add latest NFA * Upgrade other deps in example app * Ignore type error * Use supported version of react-firebaseui * Upgrade additional example app dependencies (#479) * fix typo. add missing "b" to README.md (#485) * Support firebase-admin v11 peer dependency (#504) * Upgrade dependencies [v0.x] (#505) * Upgrade some deps * Upgrade Prettier * Upgrade dependencies * Upgrade firebase-admin and copy-webpack-plugin * Upgrade example deps [v0.x] (#507) * Upgrade deps * Upgrade deps * Use compatible react-firebaseui * 0.14.4-alpha.1 * Bump NFA in package.json * Upgrade NFA in example (#508) * 0.14.4 * Use NFA 0.14.4 in example app (#509) * Add info about NextAuth.js to README [v1.x] * Update README.md * v0.x: Update bug issue template (#542) * v0.x: allow Next v13 peer dependency (#588) * 0.15.0 * Update README.md * docs: adds missing import to withAuthUserTokenSSR example * fix: make sure Firebase admin is initialized in getUserFromCookies * docs: tenantId commented by default * fix: correct attribute name to tenantId in deserializedUser * fix: extract tenantId from firebaseClientInitConfig * tests: add tenantId tests for createAuthUser. adds tests for tenantId in initFirebaseClientSDK. * merge fixes * merge fixes * merge fixes * merge fixes * fix: firebaseAdmin test. use getAuth * remove extra changes from docs * fix doc formatting * fix minor issues * empy line * fix: change auth instance * fix: remove admin import * fix: move tenantId outside firebaseClientInitConfig. Fix typos * Update src/__tests__/firebaseAdmin.test.ts --------- Co-authored-by: Kevin Jennison <kevin.jennison1@gmail.com> Co-authored-by: Guilherme <guiilherme.bayer@gmail.com> Co-authored-by: Scott Prue <prescottprue@users.noreply.github.com> Co-authored-by: Jesse Anderson <jeryanders@gmail.com> Co-authored-by: Zino Hofmann <zino@hofmann.amsterdam> Co-authored-by: Alexander Cai <alexandercai@outlook.com> Co-authored-by: Faris Abusada <abusada@users.noreply.github.com> Co-authored-by: Vinny <vpaladino778@gmail.com> Co-authored-by: Jesse Anderson <jesse.anderson@sideinc.com> Co-authored-by: camilo-mujica <84539709+camilo-mujica@users.noreply.github.com> Co-authored-by: Hegar Garcia <hegargarcia@gmail.com> Co-authored-by: Hung Vu <hunghvu2017@gmail.com> Co-authored-by: nori-k <norikatsu.kamiya@gmail.com>

hunghvu added 2 commits March 15, 2022 15:51

Add useFirebaseAdminDefaultCredential type definition

8c9941d

Fix typos, grammar, and clarify Google default credentials usage

99db330

kmjennison requested changes Mar 18, 2022

View reviewed changes

docs: Fix grammar, remove type addition from PR

40d06fc

hunghvu changed the title ~~Define useFirebaseAdminDefaultCredential for TypeScript projects and improve README~~ Fix README typos, grammar Mar 18, 2022

hunghvu mentioned this pull request Mar 18, 2022

Add useFirebaseAdminDefaultCredential type definition #451

Merged

hunghvu requested a review from kmjennison March 18, 2022 19:07

vercel bot deployed to Preview April 1, 2022 16:29 View deployment

kmjennison approved these changes Apr 1, 2022

View reviewed changes

kmjennison merged commit d9cfa82 into gladly-team:main Apr 1, 2022

kmjennison pushed a commit that referenced this pull request Apr 1, 2022

Cherry pick PR #448 onto v1.x

05ae611

kmjennison mentioned this pull request Apr 1, 2022

Update README #454

Merged

kmjennison added a commit that referenced this pull request Apr 1, 2022

Cherry pick PR #448 onto v1.x (#454)

3b23569

Co-authored-by: Hung Vu <hunghvu2017@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix README typos, grammar #448

Fix README typos, grammar #448

hunghvu commented Mar 15, 2022 •

edited

Loading

vercel bot commented Mar 15, 2022

kmjennison left a comment

kmjennison Mar 18, 2022

kmjennison Mar 18, 2022

kmjennison Mar 18, 2022

kmjennison Mar 18, 2022

hunghvu Mar 18, 2022

kmjennison Mar 18, 2022

kmjennison Mar 18, 2022

kmjennison Mar 18, 2022

vercel bot commented Apr 1, 2022 •

edited

Loading

codecov bot commented Apr 1, 2022

kmjennison commented Apr 1, 2022

kmjennison left a comment

Fix README typos, grammar #448

Fix README typos, grammar #448

Conversation

hunghvu commented Mar 15, 2022 • edited Loading

vercel bot commented Mar 15, 2022

kmjennison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Apr 1, 2022 • edited Loading

codecov bot commented Apr 1, 2022

Codecov Report

kmjennison commented Apr 1, 2022

kmjennison left a comment

Choose a reason for hiding this comment

hunghvu commented Mar 15, 2022 •

edited

Loading

vercel bot commented Apr 1, 2022 •

edited

Loading