fix: correctly detect default values including false in template parameters #1769
Conversation
lightning-sagar
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
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.
|
WalkthroughChanged default-detection in the template config loader: parameters are now treated as having a declared default if they explicitly include a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
Note: After fixing this and changing the value from the string "false" to the boolean false, the CSS/JS will generate correctly🫡 By using |
|
@lightning-sagar I reviewed the changes you made. Using: We avoid using obj.hasOwnProperty() directly because it can throw errors if the object does not inherit from Object.prototype. So to Avoid crashing and error we can use explicit version of js something like 
|
|
yes, that makes sense. your solution is definitely more robust. update: I switched to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/generator/lib/templates/config/loader.js (1)
48-56: Default detection fix looks correct; consider small cleanup andObject.hasOwnif supportedThe switch to
Object.prototype.hasOwnProperty.call(parameters[key], "default")correctly treats any explicitly declareddefault(includingfalse,0,'',null,undefined) as a default and fixes the originalfalse-is-ignored bug. 👍Two small, optional refinements:
- Normalize
parametersonce to avoid repeating|| {}and slightly tighten the types:function loadDefaultValues(templateConfig, templateParams) { - const parameters = templateConfig.parameters; - const defaultValues = Object.keys(parameters || {}).filter(key => Object.prototype.hasOwnProperty.call(parameters[key],"default")); + const parameters = templateConfig.parameters || {}; + const defaultValues = Object.keys(parameters).filter(key => + Object.prototype.hasOwnProperty.call(parameters[key], 'default') + );
- If your minimum Node/JS runtime supports it and you want to satisfy Sonar, you can switch to
Object.hasOwnin both places:- const defaultValues = Object.keys(parameters).filter(key => - Object.prototype.hasOwnProperty.call(parameters[key], 'default') - ); + const defaultValues = Object.keys(parameters).filter(key => + Object.hasOwn(parameters[key], 'default') + ); - defaultValues.filter(dv => !Object.prototype.hasOwnProperty.call(templateParams, dv)).forEach(dv => + defaultValues.filter(dv => !Object.hasOwn(templateParams, dv)).forEach(dv => Object.defineProperty(templateParams, dv, { enumerable: true, get() { return parameters[dv].default; } }) );Please double‑check that
Object.hasOwnis available in all Node versions you support before adopting this; otherwise the current implementation is fine and you may just want to tune the static‑analysis rule instead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/generator/lib/templates/config/loader.js(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
apps/generator/lib/templates/config/loader.js
[warning] 49-49: Use 'Object.hasOwn()' instead of 'Object.prototype.hasOwnProperty.call()'.
⏰ 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 - macos-latest
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - ubuntu-latest
Co-authored-by: Lukasz Gornicki <[email protected]>
…S` websocket client (asyncapi#1747) Co-authored-by: Adi-204 <[email protected]> Co-authored-by: Adi Boghawala <[email protected]> Co-authored-by: Chan <[email protected]> Co-authored-by: Lukasz Gornicki <[email protected]>
…i#1753) Co-authored-by: kartikayy007 <[email protected]> Co-authored-by: Adi Boghawala <[email protected]> Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
…, null, and empty string
lightning-sagar
force-pushed
the
fix/template-params
branch
from
a45bc1f to
4046fa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
packages/templates/clients/websocket/javascript/template/client.js.js (2)
13-13: Consider a defensive fallback whenparams.asyncapiFileDiris absentIf
params.asyncapiFileDiris not set, this will produce an invalid"undefined/asyncapi.yaml"path. If the param isn’t guaranteed required by the template config, consider adding a simple fallback to keep the generated code usable:- const asyncapiFilepath = `${params.asyncapiFileDir}/asyncapi.yaml`; + const asyncapiFilepath = params.asyncapiFileDir + ? `${params.asyncapiFileDir}/asyncapi.yaml` + : 'asyncapi.yaml';
21-24: DependencyProvider wiring looks correct; validate asyncapi path behavior at runtimeThe
additionalDependencieswiring looks good and the ordering ensurespathis required beforeasyncapiFilepathis defined. One thing to double‑check: whenasyncapiFileDiris absolute,path.resolve(__dirname, '<abs>/asyncapi.yaml')will effectively ignore__dirnameand hard‑code the generation‑time path, which may not match where the spec lives when the generated client is consumed. Please confirm that this is the intended behavior (i.e., the spec path is expected to be stable across environments).The use of single quotes inside the generated code strings is also consistent with the existing template style. Based on learnings, ...
packages/templates/clients/websocket/javascript/.ageneratorrc (1)
18-23: Quote the default value for YAML clarity.The bare dot in
default: .should be quoted asdefault: "."to make it explicit that this is a string value representing the current directory, avoiding any potential YAML parsing ambiguity.Apply this diff:
asyncapiFileDir: description: >- Custom location of the AsyncAPI file that you provided as an input in generation. By default it is located in the root of the output directory required: false - default: . + default: "."apps/keeper/CHANGELOG.md (1)
7-7: Wrap bare URLs in angle brackets for markdown compliance.The URLs should be wrapped in angle brackets to comply with markdown linting rules (MD034).
Apply this pattern for each bare URL:
-- ced1404: Pushing of release https://github.com/asyncapi/generator/pull/1747 that failed due to pipeline issues. +- ced1404: Pushing of release <https://github.com/asyncapi/generator/pull/1747> that failed due to pipeline issues.Also applies to: 13-13, 19-19
packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js (1)
26-29: Consider adding per-operation error context.If schema compilation fails for a specific operation, the current error handling (line 33) doesn't indicate which
operationIdcaused the failure, making debugging harder.Consider wrapping each iteration:
for (const operationId of this.sendOperationsId) { - this.compiledSchemas[operationId] = await compileSchemasByOperationId(asyncapiFilepath, operationId); + try { + this.compiledSchemas[operationId] = await compileSchemasByOperationId(asyncapiFilepath, operationId); + } catch (error) { + console.error(`Error compiling schema for operation '${operationId}':`, error); + throw error; + } }packages/components/src/components/SendOperations.js (2)
74-100: Inconsistent return values from static method.When
schemasis empty/missing, the method returns{ isValid: true }(line 78). However, when schemas are provided and validation is performed, the method doesn't return anything (implicitly returnsundefined). This inconsistency could confuse callers trying to check validation results.Consider returning a consistent result object:
for(const compiledSchema of schemas){ const validationResult = validateMessage(compiledSchema, message); if (validationResult.isValid) { isValid = true; socket.send(JSON.stringify(message)); - break; + return { isValid: true }; } else { if (validationResult.validationErrors) { allValidationErrors.push(...validationResult.validationErrors); } } } + if (!isValid) { + console.error('Validation errors:', JSON.stringify(allValidationErrors, null, 2)); + return { isValid: false, errors: allValidationErrors }; + } } catch (error) { console.error('Error sending ${methodName} message:', error); + return { isValid: false, error }; } }
56-63: Instance method ignores static method return value.The instance method is marked
asyncbut doesn'tawaitthe static method call (line 62), and the result is discarded. If validation fails or sending throws, the caller has no way to know.Consider returning/propagating the result:
async ${methodName}(message){ if(!this.websocket){ throw new Error('WebSocket connection not established. Call connect() first.'); } await this.compileOperationSchemas(); const schemas = this.compiledSchemas['${methodName}']; - ${clientName}.${methodName}(message, this.websocket, schemas); + return ${clientName}.${methodName}(message, this.websocket, schemas); }apps/keeper/test/index.test.js (1)
64-70: Consider adding test fornullmessage.The test covers
undefinedmessage (line 65), but the JSDoc invalidateMessageclaims it throws for "null/undefined". Consider adding a test fornullto ensure consistent behavior, or update the implementation to handlenullexplicitly.test('should throw error if message is null', () => { expect(() => validateMessage(compiledSchema, null)).toThrow('Invalid "message" parameter'); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/components/test/components/__snapshots__/DependencyProvider.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/SendOperations.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (45)
.github/workflows/release-with-changesets.yml(4 hunks).github/workflows/update-docs-on-docs-commits.yml(1 hunks)CONTRIBUTING.md(3 hunks)Development.md(2 hunks)apps/generator/CHANGELOG.md(1 hunks)apps/generator/jest.config.js(1 hunks)apps/generator/lib/templates/config/loader.js(1 hunks)apps/generator/package.json(1 hunks)apps/keeper/CHANGELOG.md(1 hunks)apps/keeper/jest.config.js(1 hunks)apps/keeper/jest.setup.js(0 hunks)apps/keeper/package.json(2 hunks)apps/keeper/src/index.js(2 hunks)apps/keeper/src/utils.js(0 hunks)apps/keeper/test/__fixtures__/asyncapi-message-validation.yml(1 hunks)apps/keeper/test/index.test.js(2 hunks)jest.config.base.js(1 hunks)packages/components/jest.config.js(1 hunks)packages/components/package.json(0 hunks)packages/components/src/components/DependencyProvider.js(1 hunks)packages/components/src/components/SendOperations.js(1 hunks)packages/helpers/jest.config.js(1 hunks)packages/helpers/package.json(0 hunks)packages/templates/clients/kafka/java/quarkus/.ageneratorrc(1 hunks)packages/templates/clients/kafka/java/quarkus/jest.config.js(1 hunks)packages/templates/clients/kafka/java/quarkus/package.json(0 hunks)packages/templates/clients/kafka/test/integration-test/jest.config.js(1 hunks)packages/templates/clients/kafka/test/integration-test/package.json(0 hunks)packages/templates/clients/websocket/dart/jest.config.js(1 hunks)packages/templates/clients/websocket/dart/package.json(0 hunks)packages/templates/clients/websocket/java/quarkus/jest.config.js(1 hunks)packages/templates/clients/websocket/java/quarkus/package.json(0 hunks)packages/templates/clients/websocket/javascript/.ageneratorrc(1 hunks)packages/templates/clients/websocket/javascript/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js(1 hunks)packages/templates/clients/websocket/javascript/components/Constructor.js(2 hunks)packages/templates/clients/websocket/javascript/jest.config.js(1 hunks)packages/templates/clients/websocket/javascript/package.json(1 hunks)packages/templates/clients/websocket/javascript/template/client.js.js(1 hunks)packages/templates/clients/websocket/python/jest.config.js(1 hunks)packages/templates/clients/websocket/python/package.json(0 hunks)packages/templates/clients/websocket/test/integration-test/jest.config.js(1 hunks)packages/templates/clients/websocket/test/integration-test/package.json(0 hunks)packages/templates/clients/websocket/test/javascript/jest.config.js(1 hunks)packages/templates/clients/websocket/test/javascript/package.json(0 hunks)
💤 Files with no reviewable changes (11)
- apps/keeper/src/utils.js
- packages/templates/clients/websocket/test/integration-test/package.json
- packages/templates/clients/kafka/java/quarkus/package.json
- packages/helpers/package.json
- packages/templates/clients/websocket/test/javascript/package.json
- apps/keeper/jest.setup.js
- packages/templates/clients/websocket/dart/package.json
- packages/templates/clients/websocket/python/package.json
- packages/templates/clients/kafka/test/integration-test/package.json
- packages/components/package.json
- packages/templates/clients/websocket/java/quarkus/package.json
✅ Files skipped from review due to trivial changes (2)
- CONTRIBUTING.md
- packages/templates/clients/kafka/java/quarkus/.ageneratorrc
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/generator/lib/templates/config/loader.js
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-12T14:57:35.024Z
Learnt from: Adi-204
Repo: asyncapi/generator PR: 1557
File: packages/templates/clients/websocket/javascript/components/SendOperation.js:21-21
Timestamp: 2025-05-12T14:57:35.024Z
Learning: In the AsyncAPI generator templates, particularly in the JavaScript WebSocket client, single quotes are used for string literals inside template-generated code (which is itself inside backticks) to preserve template placeholders like ${variable} as literal text in the generated code output.
Applied to files:
packages/templates/clients/websocket/javascript/template/client.js.jspackages/templates/clients/websocket/javascript/.ageneratorrc
📚 Learning: 2025-04-23T09:18:38.333Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.333Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
Applied to files:
packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 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/src/components/SendOperations.js
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 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/src/components/SendOperations.js
🧬 Code graph analysis (16)
packages/templates/clients/kafka/java/quarkus/jest.config.js (10)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/websocket/test/javascript/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/websocket/javascript/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/websocket/java/quarkus/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/websocket/python/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/websocket/test/integration-test/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/websocket/javascript/components/ClientClass.js (2)
packages/templates/clients/websocket/javascript/components/Constructor.js (1)
Constructor(3-27)packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js (1)
CompileOperationSchemas(3-40)
packages/templates/clients/websocket/javascript/template/client.js.js (1)
packages/components/src/components/DependencyProvider.js (1)
DependencyProvider(106-116)
packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js (1)
packages/templates/clients/websocket/javascript/template/client.js.js (1)
sendOperations(12-12)
packages/components/src/components/SendOperations.js (2)
packages/templates/clients/websocket/javascript/template/client.js.js (1)
clientName(10-10)packages/templates/clients/websocket/python/components/ClientClass.js (1)
clientName(13-13)
packages/templates/clients/websocket/dart/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
apps/generator/jest.config.js (3)
apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)
packages/components/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
packages/templates/clients/kafka/test/integration-test/jest.config.js (11)
apps/generator/jest.config.js (1)
baseConfig(2-2)apps/keeper/jest.config.js (1)
baseConfig(1-1)packages/components/jest.config.js (1)
baseConfig(1-1)packages/helpers/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/dart/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/javascript/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/python/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
baseConfig(1-1)packages/templates/clients/websocket/test/javascript/jest.config.js (1)
baseConfig(1-1)
apps/keeper/src/index.js (1)
apps/keeper/src/utils.js (3)
asyncapi(16-16)parseAsyncAPIDocumentFromFile(12-24)parseAsyncAPIDocumentFromFile(12-24)
apps/keeper/test/index.test.js (1)
apps/keeper/src/index.js (5)
compileSchema(12-14)validateMessage(70-85)compiledSchemas(26-26)compileSchemas(22-31)compileSchemasByOperationId(41-59)
🪛 markdownlint-cli2 (0.18.1)
apps/generator/CHANGELOG.md
7-7: Bare URL used
(MD034, no-bare-urls)
13-13: Bare URL used
(MD034, no-bare-urls)
19-19: Bare URL used
(MD034, no-bare-urls)
apps/keeper/CHANGELOG.md
7-7: Bare URL used
(MD034, no-bare-urls)
13-13: Bare URL used
(MD034, no-bare-urls)
19-19: Bare URL used
(MD034, no-bare-urls)
⏰ 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 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - macos-latest
🔇 Additional comments (36)
apps/generator/package.json (1)
3-3: Verify version bump strategy aligns with release plan.The version is bumped from 2.8.4 to 2.11.0, which skips intermediate minor versions. While the CHANGELOG includes entries for 2.10.0 and 2.9.0, confirm this version bump is intentional and aligns with your release and deployment strategy.
apps/generator/CHANGELOG.md (1)
21-32: Verify the loader.js fix implementation directly in the repository.The CHANGELOG documents a 2.8.4 release with WebSocket handler improvements. However, per the PR summary, the main objective is to fix template parameter default value detection in
apps/generator/lib/templates/config/loader.js. Confirm that this file has been updated to useObject.prototype.hasOwnProperty.call(parameters[key], "default")orObject.hasOwn(parameters[key], "default")to correctly detect defined defaults including falsy values (false,0,"").packages/templates/clients/websocket/javascript/.ageneratorrc (2)
11-14: This parameter benefits from the PR fix.The
appendClientSuffixparameter withdefault: false(line 14) would have been incorrectly excluded before this PR's fix inloader.js. With the corrected default-value detection logic, this false default will now properly inject into the template context.
28-29: ThecreateAsyncapiFilehook exists and is properly exported by the@asyncapi/generator-hookspackage. It correctly handles theasyncapiFileDirparameter, allowing template users to customize the output location of the generated AsyncAPI file. The hook configuration syntax in this file is correct.Development.md (1)
5-23: Excellent addition for new contributors!The new onboarding section with the webinar recommendation provides valuable context about the Generator's architecture and development workflow. This will help new contributors understand the project more effectively before diving into development.
.github/workflows/release-with-changesets.yml (3)
32-32: Good update to use macos-latest.Using
macos-latestinstead of the pinnedmacos-13ensures the workflow automatically benefits from the latest macOS runner improvements and keeps the CI environment current.
81-84: Excellent security practice with explicit permissions.Adding explicit permissions for the release job follows GitHub Actions security best practices and implements the principle of least privilege. The
id-token: writepermission likely enables provenance or signing capabilities.
98-100: Verify the temporary action reference is intentional.The action reference uses a personal fork (
derberg/.github-asyncapi) with branchfixnodereelase. This branch name suggests it may be a temporary fix (likely "fix-node-release"), and using a personal repository fork in a production workflow is unusual.Please confirm:
- Is this change intentional and necessary for this PR?
- Should this reference be reverted to the organization repository once the fix is merged upstream?
- Is there a tracking issue for migrating back to the org repository?
.github/workflows/update-docs-on-docs-commits.yml (1)
25-30: Clean implementation of centralized node version management.The addition of the
node-versioninput sourced from workflow variables enables centralized control of the Node.js version used across workflows. The NOSONAR comment appropriately documents the intentional use of the master branch reference.packages/templates/clients/websocket/dart/jest.config.js (1)
1-13: Consistent Jest configuration following repository patterns.This Jest configuration correctly extends the base config and customizes settings for JavaScript/JSX testing with babel-jest. The pattern is consistent with other template Jest configurations throughout the repository.
packages/templates/clients/kafka/java/quarkus/jest.config.js (1)
1-13: Consistent Jest configuration following repository patterns.This Jest configuration correctly extends the base config with appropriate settings for JavaScript/JSX testing. The relative path and configuration structure align with repository standards.
packages/helpers/jest.config.js (1)
1-5: Clean and minimal Jest configuration.This configuration appropriately extends the base config without unnecessary overrides, which is suitable for the helpers package. The pattern is consistent with the repository's testing setup.
packages/components/src/components/DependencyProvider.js (1)
19-19: Ensure @asyncapi/keeper is declared as a dependency in package.json.The
@asyncapi/keeperpackage exists on npm and provides message validation functionality via thecompileSchemasByOperationIdandvalidateMessagefunctions being imported. Confirm this package is listed in the project's dependencies.packages/templates/clients/websocket/test/javascript/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration correctly extends the base config and applies appropriate overrides for JavaScript/JSX test files.
packages/templates/clients/websocket/javascript/components/ClientClass.js (2)
5-5: LGTM! Import added correctly.The new component import follows the existing pattern.
13-13: LGTM! Proper prop forwarding for schema compilation.The
sendOperationsprop is correctly passed to both theConstructor(line 13) and the newCompileOperationSchemascomponent (line 27), enabling runtime schema compilation functionality.Also applies to: 27-27
packages/templates/clients/websocket/python/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration correctly extends the base config with appropriate path depth for this package location.
apps/keeper/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration correctly extends the base config with the appropriate path for the keeper app.
packages/components/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration follows the established pattern and correctly references the base config.
packages/templates/clients/kafka/test/integration-test/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration correctly extends the base config with the appropriate path depth (7 levels) for this nested location.
packages/templates/clients/websocket/java/quarkus/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration correctly extends the base config for the Java Quarkus WebSocket client tests.
packages/templates/clients/websocket/javascript/jest.config.js (1)
1-13: LGTM! Standard Jest configuration.The configuration correctly extends the base config with the appropriate path (6 levels up) for the JavaScript WebSocket client.
jest.config.base.js (1)
12-24: LGTM! Clean abstraction for shared Jest configuration.The path computation logic correctly handles both same-level and nested consumers. The function provides centralized nimma module mappings while allowing consumers to override or extend via
options.moduleNameMapper.packages/templates/clients/websocket/test/integration-test/jest.config.js (1)
1-13: LGTM! Consistent with the established Jest configuration pattern.The configuration correctly extends the base config and adds appropriate JSX support for integration tests.
apps/generator/jest.config.js (1)
2-10: LGTM! Clean migration to base configuration pattern.The base config integration correctly preserves the nunjucks-filters mapping while delegating nimma mappings to the shared base configuration. The spread order ensures local overrides take precedence.
packages/templates/clients/websocket/javascript/components/Constructor.js (1)
19-21: LGTM! Fields correctly support schema compilation feature.The new fields (
compiledSchemas,schemasCompiled,sendOperationsId) are properly initialized and integrate with thecompileOperationSchemas()method added byCompileOperationSchemascomponent.apps/keeper/package.json (3)
35-35: LGTM! babel-jest version matches jest version.The addition of
babel-jest^27.3.1 aligns with the existingjest^27.3.1 dependency and supports the Babel transpilation configured in the babel field. The Jest configuration migration to a separatejest.config.jsfile (confirmed in relevant snippets) completes the pattern.
16-20: Verify LICENSE file exists in the repository.The
filesfield referencesLICENSE(line 19), but confirmation that this file exists in the keeper directory or at the repository root is needed before publication.
29-29: Verify that all keeper source code properly migrated to use the ajv API.The swap from
@hyperjump/json-schematoajv^8.17.1 requires code updates since these libraries have different APIs. While ajv 8.17.1 (released July 2024) contains no breaking API changes, the migration from a different JSON Schema validator library requires confirming all keeper source code has been properly refactored.Ensure:
- All
@hyperjump/json-schemaimports have been removed from keeper source- Code properly uses ajv's validation API
- Any custom validation logic has been refactored for ajv compatibility
packages/templates/clients/websocket/javascript/package.json (1)
15-17: Verify jest.config.js exists for this package.The inline Jest configuration was removed (aligning with the base config pattern), but the corresponding
jest.config.jsfile for this package is not in the review set. Please confirm thatpackages/templates/clients/websocket/javascript/jest.config.jsexists and properly extends the base configuration.apps/keeper/test/__fixtures__/asyncapi-message-validation.yml (1)
34-36: Confirm that all tests referencing this fixture have been updated for the field change.The property was renamed from
timestamptomessageIdwith a type change (string/date-time → number). Any test files that validate message payloads againstasyncapi-message-validation.ymlmust have their expectations updated to referencemessageIdinstead oftimestamp, and type assertions must expect a number value.apps/keeper/test/index.test.js (2)
29-36: LGTM!Test correctly validates a message against the compiled schema and checks both
isValidandvalidationErrorsproperties.
73-90: LGTM!Good test coverage for
compileSchemasutility, including validation that the output is an array of functions and proper error handling for invalid input types.apps/keeper/src/index.js (3)
4-4: Consider potential schema caching behavior.Ajv caches compiled schemas by their
$id(or URI). If schemas with the same$idbut different content are compiled over time, Ajv will return the cached version. This is typically fine, but worth noting if schemas evolve between calls.Verify whether schemas in your AsyncAPI documents use
$idproperties, and if so, whether schema evolution is a concern for your use case.
41-59: LGTM!The function has comprehensive error handling: validates
operationIdis a non-empty string, throws clear errors for missing operations, and gracefully handles operations with no messages. The payload filtering ensures only messages with schemas are compiled.
12-14: LGTM!Simple and effective wrapper around Ajv's compile method. The JSDoc correctly documents that it throws if Ajv cannot compile the schema.
| export function validateMessage(compiledSchema, message) { | ||
| if (message === undefined) { | ||
| throw new Error(`Invalid "message" parameter: expected a value to validate, but received ${message}`); | ||
| } |
There was a problem hiding this comment.
JSDoc/implementation mismatch for null message handling.
The JSDoc on line 68 states the function throws "When message parameter is null/undefined", but line 71 only checks for undefined. A null message would bypass this check and proceed to validation.
Either update the check to include null, or correct the JSDoc:
export function validateMessage(compiledSchema, message) {
- if (message === undefined) {
+ if (message === undefined || message === null) {
throw new Error(`Invalid "message" parameter: expected a value to validate, but received ${message}`);
}🤖 Prompt for AI Agents
In apps/keeper/src/index.js around lines 70 to 73, the implementation only
checks for undefined but the JSDoc says the function throws for null/undefined;
update the runtime check to treat null the same as undefined (e.g., check
message === undefined || message === null or message == null) and adjust the
thrown Error message if needed so it correctly reports when message is null or
undefined.
| for(const compiledSchema of schemas){ | ||
| const validationResult = validateMessage(compiledSchema, message); | ||
| if (validationResult.isValid) { | ||
| isValid = true; | ||
| socket.send(JSON.stringify(message)); | ||
| break; | ||
| } else { | ||
| if (validationResult.validationErrors) { | ||
| allValidationErrors.push(...validationResult.validationErrors); | ||
| } | ||
| } | ||
| if (!isValid) { | ||
| console.error('Validation errors:', JSON.stringify(allValidationErrors, null, 2)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: validateMessage is undefined in the generated code.
The generated JavaScript code calls validateMessage(compiledSchema, message) on line 83, but this function is never imported or defined in the generated client code. This will cause a ReferenceError at runtime when any schema validation is attempted.
You need to either:
- Import
validateMessagefrom@asyncapi/keeperin the generated client, or - Inline the validation logic directly in the generated code.
Additionally, the error logging on lines 93-95 is inside the for loop. This means validation errors are logged after each failed schema, rather than only when all schemas fail. Move the error logging outside the loop:
for(const compiledSchema of schemas){
const validationResult = validateMessage(compiledSchema, message);
if (validationResult.isValid) {
isValid = true;
socket.send(JSON.stringify(message));
break;
} else {
if (validationResult.validationErrors) {
allValidationErrors.push(...validationResult.validationErrors);
}
}
- if (!isValid) {
- console.error('Validation errors:', JSON.stringify(allValidationErrors, null, 2));
- }
}
+ if (!isValid && allValidationErrors.length > 0) {
+ console.error('Validation errors:', JSON.stringify(allValidationErrors, null, 2));
+ }🤖 Prompt for AI Agents
In packages/components/src/components/SendOperations.js around lines 82 to 96,
the code calls validateMessage(compiledSchema, message) which is not defined or
imported and therefore will throw a ReferenceError; import validateMessage from
"@asyncapi/keeper" at the top of this file (or alternatively inline the
validation logic here) and replace the call accordingly, and move the
console.error that logs allValidationErrors so it runs after the for loop (i.e.,
only when no schema set isValid to true) instead of inside the loop.
| async compileOperationSchemas() { | ||
| if (this.schemasCompiled) { | ||
| return; | ||
| } | ||
| try { | ||
| // Compile schemas for all send operations | ||
| for (const operationId of this.sendOperationsId) { | ||
| this.compiledSchemas[operationId] = await compileSchemasByOperationId(asyncapiFilepath, operationId); | ||
| } | ||
| this.schemasCompiled = true; | ||
| console.log('Schemas initialized successfully for operations:', this.sendOperationsId.join(', ')); | ||
| } catch (error) { | ||
| console.error('Error initializing schemas:', error); | ||
| } | ||
| }` |
There was a problem hiding this comment.
Critical: Undefined variable asyncapiFilepath will cause runtime error.
Line 28 references asyncapiFilepath, but this variable is not defined anywhere in the generated code. This will throw a ReferenceError when the method is called.
Additionally, compileSchemasByOperationId is called but not imported, which will also cause a ReferenceError.
Please clarify:
-
How should
asyncapiFilepathbe provided? Should it be:- A constructor parameter stored as
this.asyncapiFilepath? - Passed as a parameter to
compileOperationSchemas()? - A module-level import/constant?
- A constructor parameter stored as
-
Where is
compileSchemasByOperationIddefined, and how should it be imported in the generated client code?
🤖 Prompt for AI Agents
In
packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js
around lines 20 to 35, the method references an undefined asyncapiFilepath and
an unimported compileSchemasByOperationId; update the component to accept and
store the asyncapiFilepath (add a constructor parameter and assign to
this.asyncapiFilepath) and use that instance property inside
compileOperationSchemas, and add/import the compileSchemasByOperationId helper
at the top of the file from the correct helper module (or expose it from the
codegen runtime) so the function call resolves; ensure compileOperationSchemas
uses await compileSchemasByOperationId(this.asyncapiFilepath, operationId) and
adjust any callers if you chose to change the constructor signature.
| try { | ||
| // Compile schemas for all send operations | ||
| for (const operationId of this.sendOperationsId) { | ||
| this.compiledSchemas[operationId] = await compileSchemasByOperationId(asyncapiFilepath, operationId); | ||
| } | ||
| this.schemasCompiled = true; | ||
| console.log('Schemas initialized successfully for operations:', this.sendOperationsId.join(', ')); | ||
| } catch (error) { | ||
| console.error('Error initializing schemas:', error); | ||
| } |
There was a problem hiding this comment.
Error handling doesn't match JSDoc specification.
The JSDoc at line 18 states @throws {Error} If schema compilation fails for any operation, but the implementation catches all errors and only logs them (line 33) without re-throwing. This silently swallows errors, making debugging difficult and violating the documented contract.
Consider one of these approaches:
Option 1: Re-throw errors to match JSDoc
} catch (error) {
console.error('Error initializing schemas:', error);
+ throw error;
}Option 2: Update JSDoc to reflect actual behavior
- * @throws {Error} If schema compilation fails for any operation
+ * Logs any schema compilation errors without throwing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // Compile schemas for all send operations | |
| for (const operationId of this.sendOperationsId) { | |
| this.compiledSchemas[operationId] = await compileSchemasByOperationId(asyncapiFilepath, operationId); | |
| } | |
| this.schemasCompiled = true; | |
| console.log('Schemas initialized successfully for operations:', this.sendOperationsId.join(', ')); | |
| } catch (error) { | |
| console.error('Error initializing schemas:', error); | |
| } | |
| try { | |
| // Compile schemas for all send operations | |
| for (const operationId of this.sendOperationsId) { | |
| this.compiledSchemas[operationId] = await compileSchemasByOperationId(asyncapiFilepath, operationId); | |
| } | |
| this.schemasCompiled = true; | |
| console.log('Schemas initialized successfully for operations:', this.sendOperationsId.join(', ')); | |
| } catch (error) { | |
| console.error('Error initializing schemas:', error); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In
packages/templates/clients/websocket/javascript/components/CompileOperationSchemas.js
around lines 25 to 34, the code currently catches all errors and only logs them,
which conflicts with the JSDoc that declares the function throws on failure;
either rethrow the caught error after logging (e.g., log then throw error) so
the behavior matches the @throws contract, or update the JSDoc to remove the
@throws and document that errors are logged and suppressed; choose one approach
and implement it consistently (if rethrowing, ensure callers handle the
exception; if changing JSDoc, update the comment at line 18 to reflect that
errors are logged and not thrown).
| export function Constructor({ serverUrl, sendOperations }) { | ||
| const sendOperationsId = sendOperations.map((operation) => operation.id()); | ||
| const sendOperationsArray = JSON.stringify(sendOperationsId); |
There was a problem hiding this comment.
Add validation for sendOperations parameter.
Lines 4-5 assume sendOperations is always a valid array, but there's no validation. If sendOperations is undefined or null, line 4 will throw a TypeError. This is inconsistent with CompileOperationSchemas which checks !sendOperations || sendOperations.length === 0 (line 4 in CompileOperationSchemas.js).
Add a guard at the beginning:
export function Constructor({ serverUrl, sendOperations }) {
+ if (!sendOperations || !Array.isArray(sendOperations)) {
+ throw new Error('sendOperations must be a non-empty array');
+ }
const sendOperationsId = sendOperations.map((operation) => operation.id());
const sendOperationsArray = JSON.stringify(sendOperationsId);Alternatively, if sendOperations can legitimately be empty, handle it gracefully:
export function Constructor({ serverUrl, sendOperations }) {
- const sendOperationsId = sendOperations.map((operation) => operation.id());
+ const sendOperationsId = sendOperations?.map((operation) => operation.id()) || [];
const sendOperationsArray = JSON.stringify(sendOperationsId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function Constructor({ serverUrl, sendOperations }) { | |
| const sendOperationsId = sendOperations.map((operation) => operation.id()); | |
| const sendOperationsArray = JSON.stringify(sendOperationsId); | |
| export function Constructor({ serverUrl, sendOperations }) { | |
| if (!sendOperations || !Array.isArray(sendOperations)) { | |
| throw new Error('sendOperations must be a non-empty array'); | |
| } | |
| const sendOperationsId = sendOperations.map((operation) => operation.id()); | |
| const sendOperationsArray = JSON.stringify(sendOperationsId); |
🤖 Prompt for AI Agents
In packages/templates/clients/websocket/javascript/components/Constructor.js
around lines 3 to 5, the code assumes sendOperations is a non-null array and
calls map which will throw if sendOperations is null/undefined; add a defensive
guard at the top (e.g., treat falsy values as empty array) or explicitly
validate and throw a clear error; then derive sendOperationsId from the
validated array (empty array produces empty JSON string) so subsequent
JSON.stringify never fails.
|
@derberg I noticed coderabbit flagged a bunch of issues in files I didnt touch (Keeper, WebSocket client templates, Constructor.js, etc) looks like these came in from the recent merge into master, not from the changes in this pr before I do anything, could you let me know what you'd prefer?
just want to keep the scope clean and avoid mixing unrelated fixes |
|
6 participants
Description
This PR fixes an issue where template parameters with a default value of
false(e.g.,singleFile) were not being applied, causing them to appear asundefinedinside templates.The problem was caused by this check:
Since
falseis falsy, it was incorrectly ignored.The fix updates the condition to properly detect all default values:
How I verified the fix
Patched the generator locally and added simple debug logs in the HTML template:
https://github.com/lightning-sagar/html-template/blob/test/template-params/components/index.js#L48-L49
Ran the generator using:
Output confirmed the fix:
https://github.com/lightning-sagar/html-template/blob/test/template-params/dupa/index.html#L15-L16
(
param: false | computed: false)Related issue(s)
Fixes #1768
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.