refactor: remove old FileHeaderInfo and add shared version for both JS and Python templates #1519
Conversation
AdeebaNizam404
requested review from
derberg,
magicmatatjahu,
jonaslagoni and
asyncapi-bot-eve
as code owners
|
""" WalkthroughA new shared Changes
Sequence Diagram(s)sequenceDiagram
participant TemplateClient as Template Client (JS/Python)
participant GeneratorComponents as @asyncapi/generator-components
participant FileHeaderInfo as FileHeaderInfo
TemplateClient->>GeneratorComponents: import { FileHeaderInfo }
TemplateClient->>FileHeaderInfo: <FileHeaderInfo info, server, language="js|python" />
FileHeaderInfo-->>TemplateClient: Rendered header block with appropriate comment style
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
AdeebaNizam404
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/components/src/components/FileHeaderInfo.js (1)
8-14: Consider using a more extensible approach for language-specific formatting.While the current implementation works for JavaScript and Python, future language support might be limited by the if/else structure.
Consider using an object map for comment styles, which would be more maintainable as you add more languages:
- // Decide comment style based on language - if (language === 'python') { - commentChar = '#'; - line = commentChar + ' ' + '/'.repeat(70); - } else { - commentChar = '//'; - line = commentChar + '/'.repeat(70); - } + // Define comment styles for different languages + const commentStyles = { + python: { char: '#', separator: ' ' }, + javascript: { char: '//', separator: '' }, + // Add more languages as needed + }; + + // Default to JavaScript style if language not found + const style = commentStyles[language] || commentStyles.javascript; + commentChar = style.char; + line = commentChar + style.separator + '/'.repeat(70);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/templates/clients/websocket/javascript/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/javascript/test/components/__snapshots__/FileHeaderInfo.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/components/src/components/FileHeaderInfo.js(1 hunks)packages/components/src/index.js(1 hunks)packages/components/test/components/FileHeaderInfo.test.js(1 hunks)packages/templates/clients/websocket/javascript/components/FileHeaderInfo.js(0 hunks)packages/templates/clients/websocket/javascript/template/client.js.js(2 hunks)packages/templates/clients/websocket/python/components/FileHeaderInfo.js(0 hunks)packages/templates/clients/websocket/python/template/client.py.js(2 hunks)
💤 Files with no reviewable changes (2)
- packages/templates/clients/websocket/javascript/components/FileHeaderInfo.js
- packages/templates/clients/websocket/python/components/FileHeaderInfo.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/components/test/components/FileHeaderInfo.test.js (1)
packages/components/src/components/FileHeaderInfo.js (1)
FileHeaderInfo(3-45)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (8)
packages/components/src/components/FileHeaderInfo.js (1)
1-45: The implementation looks good, with a clean approach to handling different comment styles.This new shared component correctly implements conditional comment styling based on the
languageprop, using '#' for Python and '//' for other languages. The component structure is well-organized and follows React best practices with functional components and clear prop usage.packages/components/src/index.js (1)
2-2: Export of the shared component looks good.The new
FileHeaderInfocomponent is properly exported from the index file, making it available for consumption by other packages.packages/templates/clients/websocket/javascript/template/client.js.js (3)
3-3: Import correctly updated to use the shared component.The import has been properly changed to reference the shared component from the
@asyncapi/generator-componentspackage instead of a local relative path.
18-18: Language prop correctly added to the component.The
language="javascript"prop is properly added to theFileHeaderInfocomponent to ensure JavaScript-style comments are used.
15-19:✅ Verification successful
Verify consistency between implementation and usage.
The component implementation checks for
language === 'python', but here you're passinglanguage="javascript". This works because the implementation uses an else condition for non-Python languages, but it might be clearer to match exactly.Check that the language string values used in templates match exactly with the conditions in the component:
🏁 Script executed:
#!/bin/bash # Show how the FileHeaderInfo component is used in all templates echo "FileHeaderInfo usage in templates:" rg -A 3 "FileHeaderInfo" --glob "*.js.js" packages/templates/ # Compare with the conditions in the component echo -e "\nFileHeaderInfo implementation:" cat packages/components/src/components/FileHeaderInfo.js | grep -A 5 "language ==="Length of output: 1318
Consistency Confirmed: ‘javascript’ Is Handled by the Else Branch
TheFileHeaderInfocomponent only special-caseslanguage === 'python'; all other values—including"javascript"—fall into theelsebranch and use//. No changes are required unless you’d prefer to add an explicitlanguage === 'javascript'check for clarity.packages/components/test/components/FileHeaderInfo.test.js (1)
1-40: Overall test structure looks good.The test loads a fixture, parses an AsyncAPI document, and renders the component with different server configurations. The snapshots approach is appropriate for testing React components.
packages/templates/clients/websocket/python/template/client.py.js (2)
3-3: Great work on the import refactoring!The change from a local import to using the shared component from
@asyncapi/generator-componentsis a good improvement. This promotes better code reuse across the codebase.
16-20: Well implemented component usage with language prop.The
FileHeaderInfocomponent is now properly used with the newlanguage="python"prop, allowing the shared component to render the appropriate Python-style comments. This is a good refactoring that reduces duplication while maintaining language-specific functionality.
AdeebaNizam404
changed the title
|
I guess it should be closed in favour of #1520 ? |
| import path from 'path'; | ||
| import { render } from '@asyncapi/generator-react-sdk'; | ||
| import { Parser, fromFile } from '@asyncapi/parser'; | ||
| import { FileHeaderInfo } from '../../components/FileHeaderInfo'; |
There was a problem hiding this comment.
@AdeebaNizam404 change path ../../components/FileHeaderInfo to ../../src/components/FileHeaderInfo.js
| import { FileHeaderInfo } from '../../components/FileHeaderInfo'; | ||
|
|
||
| const parser = new Parser(); | ||
| const asyncapi_websocket_query = path.resolve(__dirname, '../../../../../../helpers/test/__fixtures__/asyncapi-websocket-query.yml'); |
There was a problem hiding this comment.
Change path from
../../../../../../helpers/test/__fixtures__/asyncapi-websocket-query.yml
to
../../../helpers/test/__fixtures__/asyncapi-websocket-query.yml
also update snapshot with cmd npm run test:update.
| export function FileHeaderInfo({ info, server, language }) { | ||
| let commentChar; | ||
| let line; | ||
|
|
||
| // Decide comment style based on language | ||
| if (language === 'python') { | ||
| commentChar = '#'; | ||
| line = commentChar + ' ' + '/'.repeat(70); | ||
| } else { | ||
| commentChar = '//'; | ||
| line = commentChar + '/'.repeat(70); | ||
| } | ||
|
|
There was a problem hiding this comment.
please modify it in a similar way to https://github.com/asyncapi/generator/blob/master/packages/components/src/components/models.js
since now it is part of a components library, we need to make sure:
- it is easy to extend with more languages, so language maping is needed instead of such simple if/else
- proper jsdocs with
@typedefto make sure that whoever uses such component in their code, they get nice docs rendered
example of what we got when we use Model component I referred you to
|
@AdeebaNizam404 tests are failing because snapshots are not updated just run |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
47-50: Ensure lockfile is updated
After adding"@asyncapi/generator", please runnpm installand commit the updatedpackage-lock.json(or shrinkwrap file) to keep installs reproducible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/templates/clients/websocket/javascript/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
package.json(1 hunks)packages/templates/clients/websocket/dart/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/templates/clients/websocket/dart/package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- 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
| ], | ||
| "dependencies": { | ||
| "@asyncapi/generator": "^2.6.0" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Confirm correct package and scope for new dependency
The root package.json is marked private and uses workspaces, so hoisting a runtime dependency here may be unnecessary.
- Should this dependency instead live in
packages/componentsor in each template’s ownpackage.json? - The shared component import in your templates uses
@asyncapi/generator-components; was adding@asyncapi/generatorintentional, or did you mean to depend on@asyncapi/generator-components?
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Root package.json metadata ==="
jq '{ private, workspaces, dependencies }' package.json || cat package.json | sed -n '1,20p'
echo
echo "=== Workspace package.json files ==="
fd -t f -e package.json
echo
echo "=== Search for '@asyncapi/generator' in codebase ==="
rg -n "@asyncapi/generator"
echo
echo "=== Search for '@asyncapi/generator-components' in codebase ==="
rg -n "@asyncapi/generator-components"Length of output: 28806
Critical: Remove or relocate @asyncapi/generator from the root package.json
The root manifest is private and only orchestrates workspaces—hoisting a runtime dependency here duplicates declarations in the actual consumers.
• File: package.json (lines 47–50): drop the "@asyncapi/generator": "^2.6.0" entry
• Each template under packages/templates/** and the apps/generator workspace already declares @asyncapi/generator (currently "*"). To lock on ^2.6.0, update those entries and remove the wildcard.
• If you intended to hoist to root, instead remove the per-template declarations and document the centralized version.
• Finally, confirm you meant the CLI/runtime package (@asyncapi/generator) and not the shared components library (@asyncapi/generator-components).
🤖 Prompt for AI Agents
In package.json lines 47 to 50, remove the "@asyncapi/generator" dependency from
the root manifest because it is private and uses workspaces, so runtime
dependencies should be declared in the actual consuming packages. Instead,
update the "@asyncapi/generator" version from "*" to "^2.6.0" in each template's
package.json under packages/templates/** and in apps/generator. Confirm that you
intended to depend on "@asyncapi/generator" (the CLI/runtime package) and not
"@asyncapi/generator-components" before making these changes.
There was a problem hiding this comment.
many changes not related to this PR showed up in here, like git module, git ignore and others - for example "@asyncapi/generator": "^2.6.0" should not be there in package.json
please go through your PR and changed files and make sure in PR we only see files modified to solve this PR. Also once you cleaup package.json also make sure package-lock files are also not modified in this PR
|
Description
Removed the old FileHeaderInfo.js and FileHeaderInfo.test.js from the JavaScript client folders.
Added a new, shared version of FileHeaderInfo.js in the components/src/components directory.
Added the corresponding test file FileHeaderInfo.test.js in the components/test/components directory.
Updated both the JavaScript (client.js.js) and Python (client.py.js) templates to use the new shared FileHeaderInfo.js.
This ensures that the same FileHeaderInfo.js is used in both the JavaScript and Python client templates.
Testing:
Verified that the new FileHeaderInfo.js works correctly in both templates (client.js.js and client.py.js).
All tests pass successfully.
Related issue(s)
Fixes #1447Summary by CodeRabbit
New Features
Refactor
Tests
Chores
.gitignoreconfiguration.