feat: graphql as valid graphql tag#9163
Conversation
🦋 Changeset detectedLatest commit: 71d5350 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a changeset entry (minor version bump) and updates the JavaScript file handler so Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/biome_service/src/file_handlers/javascript.rs (1)
697-704: Stale comment — update to reflect both tags.The comment
// gql\`` only documents half the branch now.✏️ Suggested tweak
- // gql`` + // gql`` or graphql`` if let Some(AnyJsExpression::JsIdentifierExpression(ident)) = tag && ident .name() .is_ok_and(|name| name.has_name("gql") || name.has_name("graphql"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/javascript.rs` around lines 697 - 704, The comment above the branch that checks AnyJsExpression::JsIdentifierExpression(ident) currently reads "// gql``" but the condition tests both has_name("gql") and has_name("graphql"); update that inline comment to reflect both tags (e.g., "// gql`` or graphql``") so it documents the branch correctly, near the ident.name() check in the file_handlers::javascript.rs code block handling tag detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/two-signs-try.md:
- Line 2: The changeset currently marks the package "@biomejs/biome" as "patch"
but this PR introduces a new feature (recognising `graphql` as an embedded
template tag), so update the changeset entry to use the "minor" change type
instead of "patch" and ensure the changeset targets the `next` release line;
specifically replace the value "patch" with "minor" for the "@biomejs/biome"
entry in the .changeset/two-signs-try.md changeset so the release tooling will
treat it as a feature for the next branch.
In `@crates/biome_service/src/file_handlers/javascript.rs`:
- Around line 699-701: Add a snapshot test that covers tagged templates using
the tag name "graphql" (i.e., graphql`...`) to mirror the existing gql`...`
tests in graphql-tag.js; update the test file (graphql-tag.js) by duplicating
the existing gql snapshot test and replacing the tag identifier with "graphql"
so the new test generates/validates the same snapshot output for graphql`...`
templates and ensures the parser branch that checks
ident.name().is_ok_and(|name| name.has_name("gql") || name.has_name("graphql"))
is exercised.
---
Nitpick comments:
In `@crates/biome_service/src/file_handlers/javascript.rs`:
- Around line 697-704: The comment above the branch that checks
AnyJsExpression::JsIdentifierExpression(ident) currently reads "// gql``" but
the condition tests both has_name("gql") and has_name("graphql"); update that
inline comment to reflect both tags (e.g., "// gql`` or graphql``") so it
documents the branch correctly, near the ident.name() check in the
file_handlers::javascript.rs code block handling tag detection.
f86fcb4 to
5e20423
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_service/src/workspace/server.tests.rs (1)
573-581: LGTM – covers all threegraphqlinvocation styles.The three variants now tested together (
gql\`,graphql(``),graphql``) give solid coverage for the new Relay-style tagged template. The cleanup moving ``); `` onto its own line is also appreciated.One optional thought: a separate, focused test for
graphql\`in isolation (without thegql` and call-style variants) would make future failures easier to diagnose, but bundling them is perfectly fine here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.tests.rs` around lines 573 - 581, The test file currently bundles three `graphql` invocation styles (gql``, graphql(``), and graphql``) together; add a focused, isolated test that only exercises the Relay-style tagged template invocation `graphql\`\`` (e.g., using the `Baz`/`PeopleCount` query) so failures specific to that form are easier to diagnose in future, or explicitly document and keep the combined test if you prefer; implement the isolated test as a new test function that imports/uses the same `Baz` query and asserts the expected parsing/behavior for `graphql\`\`` alone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_service/src/workspace/server.tests.rs`:
- Around line 573-581: The test file currently bundles three `graphql`
invocation styles (gql``, graphql(``), and graphql``) together; add a focused,
isolated test that only exercises the Relay-style tagged template invocation
`graphql\`\`` (e.g., using the `Baz`/`PeopleCount` query) so failures specific
to that form are easier to diagnose in future, or explicitly document and keep
the combined test if you prefer; implement the isolated test as a new test
function that imports/uses the same `Baz` query and asserts the expected
parsing/behavior for `graphql\`\`` alone.
Co-authored-by: Naoki Ikeguchi <me@s6n.jp>
ef81dc7 to
7c8c203
Compare
7c8c203 to
6fb84b2
Compare
Co-authored-by: Maikel van Dort <maikel.van.dort@gmail.com>
Summary
As discussed on #9121, adds supports relay's GraphQL tag by making graphql`` as valid graphql tag.
Test Plan
added testcase on
biome_service/src/workspace/server.tests.rsDocs