refactor: move QueryParamsVariables component to @asyncapi/generator-components
#1709
Conversation
Adi-204
requested review from
derberg,
magicmatatjahu and
asyncapi-bot-eve
as code owners
|
WalkthroughAdds a shared, language-and-framework-aware React component QueryParamsVariables to packages/components, exports it, updates Java Quarkus and Python websocket templates to import and use the shared component (passing language/framework), removes template-local implementations and a Python test file, and adds cross-language snapshot tests plus a fixture with ws query params. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/components/src/components/QueryParamsVariables.js (1)
101-121: Add stable keys to rendered nodes to avoid React warnings.Provide keys on siblings returned from map.
Apply:
- return queryParams.map((param) => { + return queryParams.map((param) => { const { variableDefinition, ifCondition, assignment, closing } = generateParamCode(param); + const keyBase = param[0]; - return ( - <> - <Text indent={variableDefinition.indent} newLines={variableDefinition.newLines}> + return ( + <> + <Text key={`${keyBase}-var`} indent={variableDefinition.indent} newLines={variableDefinition.newLines}> {variableDefinition.text} </Text> - <Text indent={ifCondition.indent} newLines={ifCondition.newLines}> + <Text key={`${keyBase}-if`} indent={ifCondition.indent} newLines={ifCondition.newLines}> {ifCondition.text} </Text> - <Text indent={assignment.indent} newLines={assignment.newLines}> + <Text key={`${keyBase}-assign`} indent={assignment.indent} newLines={assignment.newLines}> {assignment.text} </Text> - {closing && ( - <Text indent={closing.indent} newLines={closing.newLines}> + {closing && ( + <Text key={`${keyBase}-close`} indent={closing.indent} newLines={closing.newLines}> {closing.text} </Text> )} </> ); });packages/templates/clients/websocket/java/quarkus/components/Constructor.js (1)
16-19: Remove redundant conditional and stabilize formatting of params initialization.This block only renders when queryParamsArray has entries (early return above), so the ternary is unnecessary. Also add newLines for consistent output.
- <Text indent={6} > - {`${ queryParamsArray && queryParamsArray.length > 0 ? 'params = new HashMap<>(); ' : ''}` - } - </Text> + <Text indent={6} newLines={1}> + {'params = new HashMap<>();'} + </Text>packages/templates/clients/websocket/python/components/Constructor.js (1)
31-37: Align param initialization with actual presence of entries (consistency with Java).Currently params = {} emits when query exists, even if it has 0 entries. Not harmful, but inconsistent with Java and slightly noisier output.
- ${ query ? 'params = {}' : ''}` + ${ queryParamsArray && queryParamsArray.length > 0 ? 'params = {}' : ''}` } </Text> <QueryParamsVariables language="python" queryParams={queryParamsArray} />packages/components/test/components/QueryParamsVariables.test.js (2)
18-29: Stabilize snapshots by sorting query param keys.Order from Map construction can vary; sorting avoids flaky snapshots.
- const queryParamsArray = queryParamsObject ? Array.from(queryParamsObject.entries()) : []; + const queryParamsArray = queryParamsObject + ? Array.from(queryParamsObject.entries()).sort(([a],[b]) => a.localeCompare(b)) + : [];
31-43: DRY the query params extraction in tests.Minor duplication across tests; a small helper keeps tests tidy.
- test('renders java quarkus query params correctly with query parameters', () => { - const channels = parsedAsyncAPIDocument.channels(); - const queryParamsObject = getQueryParams(channels); - const queryParamsArray = queryParamsObject ? Array.from(queryParamsObject.entries()) : []; + const buildQueryParamsArray = () => { + const channels = parsedAsyncAPIDocument.channels(); + const queryParamsObject = getQueryParams(channels); + return queryParamsObject + ? Array.from(queryParamsObject.entries()).sort(([a],[b]) => a.localeCompare(b)) + : []; + }; + + test('renders java quarkus query params correctly with query parameters', () => { + const queryParamsArray = buildQueryParamsArray(); const result = render( <QueryParamsVariables language='java' framework='quarkus' queryParams={queryParamsArray} /> );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
packages/components/test/components/__snapshots__/QueryParamsVariables.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/components/__snapshots__/QueryParamsVariables.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
packages/components/src/components/QueryParamsVariables.js(1 hunks)packages/components/src/index.js(1 hunks)packages/components/test/__fixtures__/asyncapi-v3.yml(1 hunks)packages/components/test/components/QueryParamsVariables.test.js(1 hunks)packages/templates/clients/websocket/java/quarkus/components/Constructor.js(2 hunks)packages/templates/clients/websocket/java/quarkus/components/QueryParamsVariables.js(0 hunks)packages/templates/clients/websocket/python/components/Constructor.js(2 hunks)packages/templates/clients/websocket/python/components/QueryParamsVariables.js(0 hunks)packages/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js(0 hunks)
💤 Files with no reviewable changes (3)
- packages/templates/clients/websocket/python/components/QueryParamsVariables.js
- packages/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js
- packages/templates/clients/websocket/java/quarkus/components/QueryParamsVariables.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
PR: asyncapi/generator#1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe
3. Messages are defined under a 'messages' container in channels
4. Servers can use a 'host' property
Applied to files:
packages/components/test/__fixtures__/asyncapi-v3.yml
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
PR: asyncapi/generator#1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with action property (send/receive) and references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe operations
3. Messages are defined under a 'messages' container in channels
4. The specification decouples operations, channels and messages to improve reusability
5. Servers can use a 'host' property
Applied to files:
packages/components/test/__fixtures__/asyncapi-v3.yml
🧬 Code graph analysis (4)
packages/components/test/components/QueryParamsVariables.test.js (3)
packages/templates/clients/websocket/java/quarkus/components/Constructor.js (1)
queryParamsArray(7-7)packages/templates/clients/websocket/python/components/Constructor.js (1)
queryParamsArray(7-7)packages/components/src/components/QueryParamsVariables.js (1)
QueryParamsVariables(94-123)
packages/templates/clients/websocket/java/quarkus/components/Constructor.js (3)
packages/components/src/components/QueryParamsVariables.js (1)
QueryParamsVariables(94-123)packages/templates/clients/websocket/java/quarkus/components/ClientConnector.js (1)
queryParamsArray(6-6)packages/templates/clients/websocket/java/quarkus/components/ClientFields.js (1)
queryParamsArray(6-6)
packages/components/src/components/QueryParamsVariables.js (1)
packages/helpers/src/utils.js (1)
toCamelCase(78-82)
packages/templates/clients/websocket/python/components/Constructor.js (2)
packages/components/src/components/QueryParamsVariables.js (1)
QueryParamsVariables(94-123)packages/templates/clients/websocket/java/quarkus/components/Constructor.js (1)
queryParamsArray(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (10)
packages/components/test/__fixtures__/asyncapi-v3.yml (1)
12-28: WS channel binding with query schema looks correct.Schema structure and defaults align with AsyncAPI 3 + WebSocket channel binding expectations.
packages/components/src/components/QueryParamsVariables.js (1)
24-41: No missing Python imports or params initialization
The Python client templates already emitimport osvia Requires.js and initializeparams = {}in Constructor.js when query parameters are used—no updates needed.Likely an incorrect or invalid review comment.
packages/components/src/index.js (1)
6-7: Export looks good.Public API wiring for QueryParamsVariables is correct.
packages/templates/clients/websocket/java/quarkus/components/Constructor.js (2)
20-24: Props passed to QueryParamsVariables look correct.language="java" and framework="quarkus" align with the shared component API.
4-4: Dependency wiring verified: No stale local imports of QueryParamsVariables;@asyncapi/generator-componentsis declared at v0.2.0 in the template’s package.json.packages/templates/clients/websocket/python/components/Constructor.js (2)
34-37: Props passed to QueryParamsVariables look correct.language="python" is sufficient here.
2-2: Dependency wiring verified
No localQueryParamsVariablesimports remain, and@asyncapi/generator-components@0.2.0 is declared in package.json.packages/components/test/components/QueryParamsVariables.test.js (3)
1-6: Solid test scaffolding.Imports and render setup look good.
13-17: Efficient parse strategy.Parsing once in beforeAll is optimal.
45-62: Good negative-case coverage.Null, undefined, and empty-array cases are covered.
@derberg so you mean like just adding JS in |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/components/src/components/QueryParamsVariables.js (3)
24-41: Python fallback must not override valid falsey values; also sanitize env var name.Use an explicit None-check and a sanitized env key so False/0/"" are preserved and env names remain POSIX-safe.
python: (param) => { - const paramName = param[0]; + const rawName = param[0]; + const envKey = rawName.replace(/[^A-Za-z0-9_]/g, '_').toUpperCase(); return { variableDefinition: { - text: `${paramName} = ${paramName} or os.getenv("${paramName.toUpperCase()}")`, + text: `${rawName} = ${rawName} if ${rawName} is not None else os.getenv("${envKey}")`, indent: 8, }, ifCondition: { - text: `if ${paramName} is not None:`, + text: `if ${rawName} is not None:`, indent: 8, }, assignment: { - text: `params["${paramName}"] = ${paramName}`, + text: `params["${rawName}"] = ${rawName}`, indent: 10, }, closing: null, }; },To ensure generated Python compiles, verify the Python template emits
import os:#!/bin/bash # Verify Python websocket constructor/template includes 'import os' rg -n -C2 --glob 'packages/templates/**/python/**' -S '\bimport os\b'
43-65: Java (Quarkus): fix env var key, params key, and guard against empties.Only the field should be camelCase; env var and query key should use the raw name. Also avoid inserting empty strings.
java: { quarkus: (param) => { - const paramName = toCamelCase(param[0]); + const rawName = param[0]; + const fieldName = toCamelCase(rawName); + const envKey = rawName.replace(/[^A-Za-z0-9_]/g, '_').toUpperCase(); return { variableDefinition: { - text: `this.${paramName} = (${paramName} != null && !${paramName}.isEmpty()) ? ${paramName} : System.getenv("${paramName.toUpperCase()}");`, + text: `this.${fieldName} = (${fieldName} != null && !${fieldName}.isEmpty()) ? ${fieldName} : System.getenv("${envKey}");`, indent: 6, }, ifCondition: { - text: `if (this.${paramName} != null){`, + text: `if (this.${fieldName} != null && !this.${fieldName}.isEmpty()){`, indent: 6, }, assignment: { - text: `params.put("${paramName}", this.${paramName});`, + text: `params.put("${rawName}", this.${fieldName});`, indent: 8, }, closing: { text: '}', indent: 6, newLines: 1, }, }; },
97-105: Fail fast for unsupported language/framework in resolver.Returning undefined here crashes later. Throw a clear error as previously suggested.
function resolveQueryParamLogic(language, framework = '') { const config = queryParamLogicConfig[language]; if (typeof config === 'function') { return config; } if (framework && config[framework]) { return config[framework]; } + throw new Error(`[QueryParamsVariables] Unsupported language "${language}"${framework ? ` and framework "${framework}"` : ''}.`); }
🧹 Nitpick comments (1)
packages/components/src/components/QueryParamsVariables.js (1)
122-143: Add a stable key to each mapped fragment.Prevents React key warnings in generator runtime.
- return queryParams.map((param) => { + return queryParams.map((param) => { + const rawNameForKey = Array.isArray(param) ? param[0] : String(param); const { variableDefinition, ifCondition, assignment, closing } = generateParamCode(param); - return ( - <> + return ( + <React.Fragment key={rawNameForKey}> <Text indent={variableDefinition.indent} newLines={variableDefinition.newLines}> {variableDefinition.text} </Text> <Text indent={ifCondition.indent} newLines={ifCondition.newLines}> {ifCondition.text} </Text> <Text indent={assignment.indent} newLines={assignment.newLines}> {assignment.text} </Text> {closing && ( <Text indent={closing.indent} newLines={closing.newLines}> {closing.text} </Text> )} - </> + </React.Fragment> ); });Add this import at the top of the file:
import React from 'react';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/components/test/components/__snapshots__/QueryParamsVariables.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/components/src/components/QueryParamsVariables.js(1 hunks)packages/components/test/components/QueryParamsVariables.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/test/components/QueryParamsVariables.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/src/components/QueryParamsVariables.js (1)
packages/helpers/src/utils.js (1)
toCamelCase(78-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - ubuntu-latest
🔇 Additional comments (1)
packages/components/src/components/QueryParamsVariables.js (1)
24-86: Align “include param” semantics across languages.Python/JS changes include empty strings and 0/false; Java excludes empty strings. Confirm desired cross-language behavior and adjust consistently.
If the intent is “include when not null/undefined”:
- Python: keep “is not None”.
- Java: drop
!isEmpty().- JS: use
!== undefined && !== null.Also applies to: 43-65
|
|
@derberg ready for review! |
derberg
changed the title
QueryParamsVariables component to @asyncapi/generator-componentsQueryParamsVariables component to @asyncapi/generator-components
|
@Adi-204 I meant enabling query params support in JS template - but yeah, not in scope here /rtm |
Description
Add new component
QueryParamsVariablesand refactor existing codebase.Related issue(s)
Fixex #1704
Summary by CodeRabbit
New Features
Refactor
Tests
Chores