-
Notifications
You must be signed in to change notification settings - Fork 910
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
Simplify check for reference doc changes #7014
Conversation
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
833a105
to
d47ad01
Compare
@@ -96,6 +96,7 @@ packages/app-check-interop-types @hsubox76 @firebase/jssdk-global-approvers | |||
# Documentation Changes | |||
packages/firebase/index.d.ts @egilmorez @firebase/jssdk-global-approvers | |||
scripts/docgen/content-sources/ @egilmorez @firebase/jssdk-global-approvers | |||
docs-devsite/ @egilmorez @markarndt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Mark has write access to this repo? cc: @markarndt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added him, let me know if any other tech writers ought to be added.
common/api-review/auth.api.md
Outdated
@@ -647,6 +653,9 @@ export interface ReactNativeAsyncStorage { | |||
setItem(key: string, value: string): Promise<void>; | |||
} | |||
|
|||
// @public | |||
export const reactNativeLocalPersistence: Persistence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does getReactNativePersistence
also need to be exported here? I think it's exported out of index.doc.d.ts alongside reactNativeLocalPersistence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just removed this file from the PR.
packages/auth/api-extractor.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"extends": "../../config/api-extractor.json", | |||
"mainEntryPointFilePath": "<projectFolder>/dist/esm5/index.d.ts", | |||
"mainEntryPointFilePath": "<projectFolder>/dist/esm5/index.doc.d.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have enough context around why we had index.d.ts as the main entry point instead of index.doc.d.ts - maybe @sam-gc to weigh in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is an artifact of the doc generation process and shouldn't be checked in. Removing it.
d47ad01
to
01654a7
Compare
5298827
to
64fae79
Compare
The plan is:
docs-devsite/
in the repo. (This will be done in a future PR because it is 189 files and will make this PR difficult to review.)check-docs.yml
. This will run new doc generation and do agit-diff
. If there are any changes it will fail and print a message asking the user to runyarn docgen devsite
locally.docs-devsite/
directory.