-
Notifications
You must be signed in to change notification settings - Fork 915
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
Spelling #8280
Spelling #8280
Conversation
🦋 Changeset detectedLatest commit: 8433b90 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -38,7 +38,7 @@ export declare class DataSnapshot | |||
| [child(path)](./database.datasnapshot.md#datasnapshotchild) | | Gets another <code>DataSnapshot</code> for the location at the specified relative path.<!-- -->Passing a relative path to the <code>child()</code> method of a DataSnapshot returns another <code>DataSnapshot</code> for the location at the specified relative path. The relative path can either be a simple child name (for example, "ada") or a deeper, slash-separated path (for example, "ada/name/first"). If the child location has no data, an empty <code>DataSnapshot</code> (that is, a <code>DataSnapshot</code> whose value is <code>null</code>) is returned. | | |||
| [exists()](./database.datasnapshot.md#datasnapshotexists) | | Returns true if this <code>DataSnapshot</code> contains any data. It is slightly more efficient than using <code>snapshot.val() !== null</code>. | | |||
| [exportVal()](./database.datasnapshot.md#datasnapshotexportval) | | Exports the entire contents of the DataSnapshot as a JavaScript object.<!-- -->The <code>exportVal()</code> method is similar to <code>val()</code>, except priority information is included (if available), making it suitable for backing up your data. | | |||
| [forEach(action)](./database.datasnapshot.md#datasnapshotforeach) | | Enumerates the top-level children in the <code>IteratedDataSnapshot</code>.<!-- -->Because of the way JavaScript objects work, the ordering of data in the JavaScript object returned by <code>val()</code> is not guaranteed to match the ordering on the server nor the ordering of <code>onChildAdded()</code> events. That is where <code>forEach()</code> comes in handy. It guarantees the children of a <code>DataSnapshot</code> will be iterated in their query order.<!-- -->If no explicit <code>orderBy*()</code> method is used, results are returned ordered by key (unless priorities are used, in which case, results are returned by priority). | | |||
| [forEach(action)](./database.datasnapshot.md#datasnapshotforeach) | | Enumerates the top-level children in the <code>IteratedDataSnapshot</code>.<!-- -->Because of the way JavaScript objects work, the ordering of data in the JavaScript object returned by <code>val()</code> is neither guaranteed to match the ordering on the server nor the ordering of <code>onChildAdded()</code> events. That is where <code>forEach()</code> comes in handy. It guarantees the children of a <code>DataSnapshot</code> will be iterated in their query order.<!-- -->If no explicit <code>orderBy*()</code> method is used, results are returned ordered by key (unless priorities are used, in which case, results are returned by priority). | |
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'm happy to drop any of my proposed changes.
neither/nor is the generally recommended way to write this, and as this is documentation, using it seems reasonable.
@@ -2598,7 +2598,7 @@ export declare type OrderByDirection = 'desc' | 'asc'; | |||
|
|||
## PartialWithFieldValue | |||
|
|||
Similar to Typescript's `Partial<T>`<!-- -->, but allows nested fields to be omitted and FieldValues to be passed in as property values. | |||
Similar to TypeScript's `Partial<T>`<!-- -->, but allows nested fields to be omitted and FieldValues to be passed in as property values. |
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.
brand
@@ -10,7 +10,7 @@ | |||
* | |||
* Unless required by applicable law or agreed to in writing, software | |||
* distributed under the License is distributed on an "AS IS" BASIS, | |||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or ied. | |||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. |
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.
generally, lawyers get annoyed by changes to licenses...
A peer suggested that someone did a global search and replace s/impl//
. -- I believe they're correct.
@@ -518,7 +518,7 @@ ul.tsd-descriptions.active > .tsd-description.fade-out { -webkit-animation: fade | |||
ul.tsd-descriptions h4, ul.tsd-descriptions .tsd-index-panel h3, .tsd-index-panel ul.tsd-descriptions h3 { font-size: 16px; margin: 1em 0 0.5em 0; } | |||
|
|||
ul.tsd-parameters, ul.tsd-type-parameters { list-style: square; margin: 0; padding-left: 20px; } | |||
ul.tsd-parameters > li.tsd-parameter-siganture, ul.tsd-type-parameters > li.tsd-parameter-siganture { list-style: none; margin-left: -20px; } | |||
ul.tsd-parameters > li.tsd-parameter-signature, ul.tsd-type-parameters > li.tsd-parameter-signature { list-style: none; margin-left: -20px; } |
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.
notable
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.
It's unclear if this CSS class name change was also made in the corresponding HTML. If not, maybe revert this?
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.
There's no sign of that typo in html https://github.com/search?q=repo%3Afirebase%2Ffirebase-js-sdk%20%22siganture%22&type=code
Or the attribute for that matter? https://github.com/search?q=repo%3Afirebase%2Ffirebase-js-sdk+%22tsd-parameter%22&type=code
It appears to be from something else that isn't used here..
https://github.com/search?q=repo%3Aangular-redux%2Fstore%20%22tsd-parameter-siganture%22&type=code
Happy to drop. I'll probably try to send a PR to them at some point...
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.
-snasphot_listener_source.test.ts
+snapshot_listener_source.test.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.
-inheritence.input.d.ts
+inheritance.input.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.
This is a super useful effort that I enthusiasticallyt endorse (could the same tooling be used on similar TypeScript source, like the Cloud Functions reference?), but wow – over 200 files is a lot to review at once, for a human. Next time around, would be great to break it down into categories of some kind, like proper naming issues (Github> GitHub), misspellings of nouns, misspellings of verbs, whatever.
Also, for some of the more sophisticated errata, we may need to tweak the algorithm to prevent the neither/nor errors that I picked out. Hope I didn't miss any similar things ( placement of "only" often causes similar issues).
The changes are all manual (more or less), and anything that isn't a simple replacement is very manual (especially things like Splitting is more or less possible -- it's easy for me to group commits together, or split by directory. It's fairly painful for me to split by part of speech if a word is used in more than one part of speech (and gets prohibitive as the word count grows -- it's error prone, and as a human, I make mistakes like everyone else), but it's certainly not a problem for me to peel off individual commits or group commits together into separate PRs -- I just need direction. For smaller repositories, I can split by "code" vs "comment" -- although I really don't like to do it as it isn't particularly safe -- it's much cleaner to split by directory or possibly file extension. The only thing that's fully automated is identifying potential problems. I can certainly run the tooling on any repository if people are interested. Sometimes I rely on Google Sheets for word corrections, although of late, I've just been using Google Chrome suggestions. |
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 think jsoref fixed or backed out content related to my neither/nor suggestions (not easy to immediately discover that due to the massive size of this PR), so I think I'm happy to approve. Thanks!
Do I need to worry about the changeset bot? Note that https://github.com/firebase/firebase-js-sdk/actions/runs/9639724064/job/26582355096?pr=8280 is failing (I can't remember the state of this, I split most other CI failures into other PRs...) |
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 with comments. I reviewed packages/firestore
and other top-level folders outside of packages/
.
@@ -518,7 +518,7 @@ ul.tsd-descriptions.active > .tsd-description.fade-out { -webkit-animation: fade | |||
ul.tsd-descriptions h4, ul.tsd-descriptions .tsd-index-panel h3, .tsd-index-panel ul.tsd-descriptions h3 { font-size: 16px; margin: 1em 0 0.5em 0; } | |||
|
|||
ul.tsd-parameters, ul.tsd-type-parameters { list-style: square; margin: 0; padding-left: 20px; } | |||
ul.tsd-parameters > li.tsd-parameter-siganture, ul.tsd-type-parameters > li.tsd-parameter-siganture { list-style: none; margin-left: -20px; } | |||
ul.tsd-parameters > li.tsd-parameter-signature, ul.tsd-type-parameters > li.tsd-parameter-signature { list-style: none; margin-left: -20px; } |
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.
It's unclear if this CSS class name change was also made in the corresponding HTML. If not, maybe revert this?
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.
Hi @jsoref, thanks for the PR!
Could you add a changeset (run yarn changeset
) and mark all packages with code changes as patch
with the note Fixed typos in documentation and in some internal variables and parameters.
?
Also looks like you have a format error. Could your run the following commands:
yarn format
yarn docgen:all
You might need to merge the main branch into your branch because there were some changes to the documentation script a few weeks ago.
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
@DellaBitta: I can't get yarn to cooperate jsoref@jsoref-mbp firebase-js-sdk % node --version
v20.11.0
jsoref@jsoref-mbp firebase-js-sdk % yarn
➤ YN0000: · Yarn 4.2.1
➤ YN0000: ┌ Resolution step
➤ YN0001: │ Error: closure-net@https://github.com/google/closure-net.git#commit=0412666e8f29b8ae69decb1fdc7ead635a5cf43e: Manifest not found
at AE.find (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:140:120021)
at async /Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:520:11979
at async Object.xZe (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:140:53847)
at async I2.resolve (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:520:11940)
at async Dd.resolve (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:141:1451)
at async Dd.resolve (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:141:1451)
at async /Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:210:7249
at async Ky (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:140:53910)
at async ne (/Users/jsoref/.yarn/releases/yarn-4.2.1.cjs:210:7231)
at async Promise.allSettled (index 1)
➤ YN0000: └ Completed in 0s 873ms
➤ YN0000: · Failed with errors in 0s 883ms I've given permission to push into my branch... |
Done! Waiting for CI to execute now. |
Numerous fixes to spelling across the SDK.
Discussion
Fixes #8279
Testing
API Changes
I believe there's one API change included:
jsoref@10eacae
I've included it in the hopes that it isn't an API and, thus, can be changed. If it's an API and can't be changed, it's trivial for me to drop it (just as I'm more than happy to drop any other changes).