feat: adjusts GraphQL caching logic#3295
Open
eduardoformiga wants to merge 2 commits intocanary-v2from
Open
Conversation
- Introduced `recast` as a new dependency in the CLI package. - Refactored the GraphQL caching command to utilize `recast` for updating cached operations while preserving formatting and comments in the configuration file. - Removed the previous usage of `prettier` for formatting within the caching logic, replacing it with a more robust solution that integrates `recast` for AST manipulation.
eduardoformiga
requested review from
gabpaladino and
lemagnetic
and removed request for
a team
Contributor
WalkthroughAdded ChangesAST-Based Config Patching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/cache-graphql.ts`:
- Around line 217-225: The parser currently returns false when it doesn't find
an 'experimental' property on node.right; instead, if node.right is an
ObjectExpression, synthesize a new 'experimental' ObjectExpression property
(using the same AST builders where other properties are created) and assign it
to node.right.properties so downstream code can mutate it; only keep the
hard-fail when node.right is not an ObjectExpression (i.e., module.exports is
not an object). Apply the same change to the second occurrence that checks
experimental (the block around the other isPropertyNamed/experimentalProp
check).
- Around line 255-283: The removeRecastBlankLinesInExperimental text-based
cleanup is unsafe; remove its use and keep the recast/prettier output unchanged
by returning recast.print(ast).code instead of calling
removeRecastBlankLinesInExperimental, and delete the
removeRecastBlankLinesInExperimental function (or stop exporting/using it) to
avoid non-syntax-aware regex edits; locate references to
removeRecastBlankLinesInExperimental and the return that currently wraps
recast.print(ast).code and update accordingly.
- Around line 233-241: The current code replaces the whole property node when
updating cachedOperations (props[idx] = newProp), which loses Recast
metadata/comments; instead locate the existing property node (the element at
props[idx]) and mutate its value to newArray (e.g., set existingProp.value =
newArray) so comments/formatting are preserved; update the branch that pushes
newProp only when no existing property exists. Also replace broad any
annotations in this function with the correct AST types for nodes (e.g.,
Property, ArrayExpression, Identifier) to improve TypeScript safety.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b35c2fd-c24d-428b-b50b-103259f3bdc2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (2)
packages/cli/package.jsonpackages/cli/src/commands/cache-graphql.ts
Comment on lines
+217
to
+225
| const experimentalProp = node.right.properties.find((p: any) => | ||
| isPropertyNamed(p, 'experimental') | ||
| ) | ||
|
|
||
| if ( | ||
| !experimentalProp || | ||
| experimentalProp.value.type !== 'ObjectExpression' | ||
| ) { | ||
| return false |
Contributor
There was a problem hiding this comment.
Create experimental when it is missing.
This now hard-fails for any valid discovery.config that does not already declare experimental as an object. Since the command accepts a custom --config, it should synthesize that wrapper and only error when module.exports itself is not an object.
Suggested fix
- const experimentalProp = node.right.properties.find((p: any) =>
- isPropertyNamed(p, 'experimental')
- )
-
- if (
- !experimentalProp ||
- experimentalProp.value.type !== 'ObjectExpression'
- ) {
- return false
- }
+ let experimentalProp = node.right.properties.find((p: any) =>
+ isPropertyNamed(p, 'experimental')
+ )
+
+ if (!experimentalProp) {
+ experimentalProp = b.objectProperty(
+ b.identifier('experimental'),
+ b.objectExpression([])
+ )
+ node.right.properties.push(experimentalProp)
+ }
+
+ if (experimentalProp.value.type !== 'ObjectExpression') {
+ return false
+ }Also applies to: 249-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/cache-graphql.ts` around lines 217 - 225, The
parser currently returns false when it doesn't find an 'experimental' property
on node.right; instead, if node.right is an ObjectExpression, synthesize a new
'experimental' ObjectExpression property (using the same AST builders where
other properties are created) and assign it to node.right.properties so
downstream code can mutate it; only keep the hard-fail when node.right is not an
ObjectExpression (i.e., module.exports is not an object). Apply the same change
to the second occurrence that checks experimental (the block around the other
isPropertyNamed/experimentalProp check).
- Eliminated the `recast` dependency from the CLI package. - Refactored the GraphQL caching command to remove the use of `recast`, simplifying the logic for updating cached operations. - Updated the method for handling the `experimental.cachedOperations` property in the configuration file, ensuring comments and formatting are preserved without AST manipulation.
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
eduardoformiga
changed the title
eduardoformiga
requested review from
hellofanny and
ommeirelles
and removed request for
gabpaladino
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's the purpose of this pull request?
This pull request significantly improves how the CLI command updates the
cachedOperationsproperty in the GraphQL discovery config file. Instead of reconstructing and formatting the entire config object, it now performs a targeted string update of only theexperimental.cachedOperationsarray, preserving the rest of the file (including comments and formatting) and then normalizing the result with Prettier. Several helper functions were added to locate and patch the relevant block in the file.