fix: exclude asyncapi-ui.min.js from transpilation to prevent Babel deoptimisation warning #1772
Conversation
lightning-sagar
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
WalkthroughSuppresses non-debug Babel "[BABEL]" stderr output during transpilation by temporarily overriding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 |
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.
|
…vent Babel deoptimisation warning
lightning-sagar
force-pushed
the
fix/ignore-asyncapi-ui-min
branch
from
2578004 to
296272a
Compare
lightning-sagar
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: 0
🧹 Nitpick comments (2)
apps/react-sdk/src/transpiler/transpiler.ts (2)
21-36: Improve type safety for warnings array.The
any[]type loses type information. Rollup exportsRollupWarningwhich provides better type safety and IDE support.+import { rollup, RollupWarning } from 'rollup'; -import { rollup } from 'rollup';- const warnings: any[] = []; + const warnings: RollupWarning[] = [];Also, consider typing the
onwarnparameter:- onwarn: (warning) =>{ + onwarn: (warning: RollupWarning) => {
81-87: Consider consistent warning output in debug mode.There's an inconsistency in how warnings are emitted in debug mode:
- BABEL messages are forwarded directly to
stderr(line 30)- Rollup warnings are logged via
log.debug()(line 86)This could result in different output destinations depending on loglevel configuration. Consider using
log.debug()for both, or forwarding both to stderr for consistency.Additionally, if transpilation throws an error, collected warnings won't be logged. Consider moving the warning logging into the finally block or a catch block if warnings should be visible even on failure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/react-sdk/src/transpiler/transpiler.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
apps/react-sdk/src/transpiler/transpiler.ts
📚 Learning: 2025-05-12T17:29:03.886Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1557
File: packages/templates/clients/websocket/javascript/components/SendOperation.js:7-7
Timestamp: 2025-05-12T17:29:03.886Z
Learning: Do not suggest adding key props to mapped React elements in the asyncapi/generator repository, even when linters like Biome flag them as missing.
Applied to files:
apps/react-sdk/src/transpiler/transpiler.ts
🧬 Code graph analysis (1)
apps/react-sdk/src/transpiler/transpiler.ts (1)
apps/generator/lib/generator.js (1)
ROOT_DIR(44-44)
⏰ 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: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - macos-latest
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
apps/react-sdk/src/transpiler/transpiler.ts (2)
2-2: LGTM!The
loglevelimport is appropriate for consistent debug logging with the rest of the codebase.
47-79: LGTM!The rollup configuration with
onwarnhandler correctly collects warnings for deferred logging. This approach addresses the PR objective by suppressing noisy warnings by default while preserving them for debug mode.
|
@derberg ,
is this the right approach or would you prefer a different way of handling these warnings?? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/react-sdk/src/transpiler/transpiler.ts (1)
21-36: Consider passingdebugas an option parameter and hoisting stderr interception.Two concerns:
Reading
--debugfromprocess.argvcouples this SDK function directly to CLI behavior. For better reusability, passdebugvia theoptionsparameter.Each recursive call (line 92) re-intercepts stderr and creates a new warnings array, leading to nested interceptions. Hoist the interception to occur once at the top-level call.
Suggested approach:
-export async function transpileFiles(directory: string, outputDir: string, options?: TranspileFilesOptions) { +export async function transpileFiles(directory: string, outputDir: string, options?: TranspileFilesOptions & { debug?: boolean }) { const { files, dirs } = await getStatsInDir(directory); const warnings: RollupWarning[] = []; - const debug = process.argv.includes('--debug'); + const debug = options?.debug ?? false; + const isTopLevel = options?._isTopLevel !== false;Then only intercept stderr for top-level calls and pass
{ ...options, _isTopLevel: false }to recursive calls.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/react-sdk/src/transpiler/transpiler.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
apps/react-sdk/src/transpiler/transpiler.ts
📚 Learning: 2025-05-12T17:29:03.886Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1557
File: packages/templates/clients/websocket/javascript/components/SendOperation.js:7-7
Timestamp: 2025-05-12T17:29:03.886Z
Learning: Do not suggest adding key props to mapped React elements in the asyncapi/generator repository, even when linters like Biome flag them as missing.
Applied to files:
apps/react-sdk/src/transpiler/transpiler.ts
🧬 Code graph analysis (1)
apps/react-sdk/src/transpiler/transpiler.ts (2)
apps/generator/lib/generator.js (1)
ROOT_DIR(44-44)apps/react-sdk/src/transpiler/__tests__/testfiles/ES6/simple.js (1)
require(4-4)
⏰ 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: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - macos-latest
🔇 Additional comments (2)
apps/react-sdk/src/transpiler/transpiler.ts (2)
2-4: LGTM on imports.The additions of
loglevelandRollupWarningtype are appropriate for the new warning handling functionality.
85-87: Warnings from recursive calls are handled separately.Each recursive invocation creates its own
warningsarray and logs independently. This works but results in interleaved debug output. If consolidated reporting is desired later, consider aggregating warnings across recursive calls.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/react-sdk/src/transpiler/transpiler.ts (2)
25-37: Babel stderr filtering behavior: confirm that hiding all[BABEL]output is desiredThe interception correctly forwards non‑Babel writes to the original
stderrand only swallows chunks containing[BABEL], optionally re‑emitting them vialog.debugwhen--debugis passed. This matches the maintainer request to suppress Babel warnings by default.Two small points to validate:
- This will hide all Babel
[BABEL]messages (not just the deoptimisation note) in non‑debug runs. If there are other useful Babel diagnostics that also start with[BABEL], they will only be visible under--debug.- The callback handling (
if (typeof cb === 'function') cb();) ensures upstream callers aren’t left hanging, which is good; just be aware that if Babel (or another tool) relies on stderr backpressure instead of callbacks, this short‑circuiting assumes the writes are small enough not to matter.If that behavior is acceptable, the implementation looks fine as‑is.
If you later find this too broad, you could tighten the filter to specific message substrings (e.g. the deoptimisation note) or add a small allowlist of Babel warning codes while still routing them through
log.debug.
47-52: Rollup warning collection and bundle cleanup look good; consider more robustclose()on failuresNice improvements here:
onwarnnow collectsRollupWarnings into an array and only logs them at debug level, which keeps normal output clean and aligns with the--debugbehavior.await bundles.close()afterbundles.write()ensures plugins get theircloseBundlehook and resources are released on the success path.- Debug logging of
warningsvialog.debug(w.message)is straightforward and low‑noise.One potential hardening you may want to consider: if
bundles.write(...)throws,bundles.close()is never called. Per Rollup 2.60.1's JavaScript API documentation, thecloseBundlehook accepts an optionalerrorparameter—indicating that plugins expect to receive cleanup notifications even when failures occur. Wrapping thewrite/closepair intry/finallyensures plugins always get the opportunity to clean up resources:- const bundles = await rollup({ + const bundles = await rollup({ /* config */ - }) - await bundles.write({ /* options */ }); - await bundles.close(); + }); + try { + await bundles.write({ /* options */ }); + } finally { + await bundles.close(); + }Also applies to: 70-80, 86-87
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/react-sdk/src/transpiler/transpiler.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
apps/react-sdk/src/transpiler/transpiler.ts
📚 Learning: 2025-05-12T17:29:03.886Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1557
File: packages/templates/clients/websocket/javascript/components/SendOperation.js:7-7
Timestamp: 2025-05-12T17:29:03.886Z
Learning: Do not suggest adding key props to mapped React elements in the asyncapi/generator repository, even when linters like Biome flag them as missing.
Applied to files:
apps/react-sdk/src/transpiler/transpiler.ts
⏰ 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: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - macos-latest
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - ubuntu-latest
🔇 Additional comments (1)
apps/react-sdk/src/transpiler/transpiler.ts (1)
21-24: Globalprocess.stderr.writeoverride is scoped but has concurrency implicationsThe pattern of capturing
originalStderr, overridingprocess.stderr.write, and restoring it in afinallyblock properly contains the override to this function's execution. However, it still affects the entire process globally whiletranspileFilesis running. If this function is invoked in parallel from different parts of a long‑lived process, the overrides could interfere with each other or with other code hookingstderr.Not urgent for current CLI‑style usage, but consider in the future:
- Passing a
debug/suppressBabeloption into this function instead of readingprocess.argvdirectly.- Centralizing stderr interception at a higher CLI layer to keep this helper side‑effect free.
Verify that
transpileFilescall patterns across the repository confirm it cannot be invoked concurrently before treating this as a non-issue.Also applies to: 38-39, 82-84
3 participants
Description
This PR fixes the long-standing Babel warning:
The issue occurs because the React transpiler processes
asyncapi-ui.min.js, which is a large minified dependency that should not be transpiled.What was changed
Updated the React SDK transpiler to ignore
asyncapi-ui.min.jsbefore passing files to Rollup/Babel.Implemented a simple filter:
(and equivalent logic where needed)
After applying the fix, the generator runs without any Babel deoptimisation warnings.
📌 Additional note
The old repo (
asyncapi-archived-repos/generator-react-sdk) is archived, so this change was made in the active path:Related issue(s)
Fixes:
#1771
#717
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.