Conversation
🦋 Changeset detectedLatest commit: 4ef1098 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces subquery support to Firestore Pipelines, adding new expressions such as arrayFilter, arraySlice, and arrayTransform, along with subcollection and define stages. It also enables converting pipelines into expressions via toArrayExpression and toScalarExpression. Feedback identifies several areas for improvement, including restoring missing documentation for the timestampDiff function, removing a .only modifier from tests, fixing documentation formatting and typos, and ensuring naming consistency for internal method identifiers.
| // Warning: (ae-incompatible-release-tags) The symbol "timestampDiff" is marked as @public, but its signature references "Expression" which is marked as @beta | ||
| // Warning: (ae-incompatible-release-tags) The symbol "timestampDiff" is marked as @public, but its signature references "FunctionExpression" which is marked as @beta | ||
| // | ||
| // @public (undocumented) |
There was a problem hiding this comment.
The timestampDiff function has been promoted to @public but is marked as (undocumented). This has resulted in its documentation being removed from the generated doc files (e.g., docs-devsite/firestore_lite_pipelines.md). Public APIs must have complete TSDoc documentation. Please add the documentation for this function.
| expectResults(snapshot, ...expectedResults); | ||
| }); | ||
|
|
||
| it.only('supports arrayFilter', async () => { |
There was a problem hiding this comment.
This test is marked with .only, which will cause other tests in this file to be skipped. Please remove .only before merging.
| it.only('supports arrayFilter', async () => { | |
| it('supports arrayFilter', async () => { |
| ## Expression.arrayFilter() | ||
|
|
||
| > This API is provided as a preview for developers and may change based on feedback that we receive. Do not use this API in a production environment. | ||
| > |
There was a problem hiding this comment.
There's an extra blank line in the blockquote here, and in similar sections for newly added functions in the documentation files. This should be removed for cleaner formatting.
| return new FunctionExpression( | ||
| 'get_field', | ||
| [this, valueToDefaultExpr(key)], | ||
| 'get_field' |
There was a problem hiding this comment.
The method name 'get_field' is inconsistent with other method names like 'arrayFilter' or 'arraySlice', which use camelCase. For consistency, this should probably be 'getField'. This name is used for error messages.
| 'get_field' | |
| 'getField' |
| * <p>Result Unwrapping:</p> | ||
| * <ul> | ||
| * <li>If the item has a single field, its value is unwrapped and returned directly.</li> | ||
| * <li>f the item has multiple fields, they are returned as an object.</li> |
There was a problem hiding this comment.
There's a typo here. It should be "If" instead of "f".
| * <li>f the item has multiple fields, they are returned as an object.</li> | |
| * <li>If the item has multiple fields, they are returned as an object.</li> |
2 participants
Add
arraySlice,arrayFilter,arrayTransformandarrayTransformWithIndexexpressions