fix: wrap script in async IIFE to create isolated scope#6229
fix: wrap script in async IIFE to create isolated scope#6229bijin-bruno merged 2 commits intousebruno:mainfrom
Conversation
187e176 to
218b156
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughWraps each script segment (collection, folder, request) in an async IIFE to isolate scope per segment and changes assembly logic for pre-request, post-response, and test scripts, including reversed ordering for non-sequential flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
🧬 Code graph analysis (1)packages/bruno-cli/src/utils/collection.js (2)
⏰ 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)
🔇 Additional comments (4)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/bruno-cli/src/utils/collection.js (3)
285-294: Optional: Firstcompact()call is redundant.Since
wrapScriptInClosurereturns''for falsy/empty values, the secondcompact()on line 294 will handle filtering. The firstcompact()on line 289 could be removed without affecting behavior.- const preReqScripts = compact([ + const preReqScripts = [ collectionPreReqScript, ...combinedPreReqScript, request?.script?.req || '' - ]); + ]; request.script.req = compact(preReqScripts.map(wrapScriptInClosure)).join(os.EOL + os.EOL);
296-312: Optional: Minor style improvements.Two minor opportunities for cleaner code:
- The first
compact()is redundant (same reasoning as pre-request scripts).reverse()mutates the array; consider creating a copy first for immutabilityif (scriptFlow === 'sequential') { - const postResScripts = compact([ + const postResScripts = [ collectionPostResScript, ...combinedPostResScript, request?.script?.res || '' - ]); + ]; request.script.res = compact(postResScripts.map(wrapScriptInClosure)).join(os.EOL + os.EOL); } else { - const postResScripts = compact([ + const postResScripts = [ request?.script?.res || '', - ...combinedPostResScript.reverse(), + ...[...combinedPostResScript].reverse(), collectionPostResScript - ]); + ]; request.script.res = compact(postResScripts.map(wrapScriptInClosure)).join(os.EOL + os.EOL); }
314-330: Optional: Apply same refactoring as post-response scripts.Same opportunities as the post-response section: redundant
compact()and array mutation.if (scriptFlow === 'sequential') { - const testScripts = compact([ + const testScripts = [ collectionTests, ...combinedTests, request?.tests || '' - ]); + ]; request.tests = compact(testScripts.map(wrapScriptInClosure)).join(os.EOL + os.EOL); } else { - const testScripts = compact([ + const testScripts = [ request?.tests || '', - ...combinedTests.reverse(), + ...[...combinedTests].reverse(), collectionTests - ]); + ]; request.tests = compact(testScripts.map(wrapScriptInClosure)).join(os.EOL + os.EOL); }packages/bruno-electron/src/utils/collection.js (1)
182-227: Optional: Apply same refactoring suggestions as CLI version.The script wrapping logic (pre-request, post-response, tests) has the same opportunities for minor cleanup as the CLI version:
- Redundant first
compact()calls (lines 186, 195, 203, 213, 221)- Array mutation via
.reverse()(lines 205, 223)Refer to the refactoring suggestions in the CLI file for specific diff blocks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-cli/src/utils/collection.js(2 hunks)packages/bruno-electron/src/utils/collection.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-cli/src/utils/collection.jspackages/bruno-electron/src/utils/collection.js
🔇 Additional comments (4)
packages/bruno-cli/src/utils/collection.js (2)
239-254: LGTM - IIFE wrapper correctly isolates script scope.The implementation properly prevents variable re-declaration and confines early returns to each script segment, addressing the core issue from #6130.
251-253: Verify that the script execution environment supports top-level await or provides an async context for these async IIFEs.The wrapped scripts use
await (async () => { ... })(), which requires either top-level await support or execution within an already-async context. Confirm that the runner/executor evaluates these scripts appropriately.packages/bruno-electron/src/utils/collection.js (2)
135-151: LGTM - Implementation mirrors CLI version correctly.The
wrapScriptInClosurefunction and its usage are consistent across both packages, ensuring uniform behavior.
148-150: Verify async IIFE execution support in Electron's script runtime.The code wraps scripts in an async IIFE pattern (
await (async () => { ${script} })();). Confirm that Electron's script executor properly handles async/await syntax and that this pattern is consistent with the CLI implementation.
|
This is a breaking change. We should carefully consider if we should do this for v3 release. |
Description
Currently bruno concatenates all the scripts across the request tree path, so if we declare a variable within collection level script, we cannot redeclare the same variable, within any of the scripts that comes after it.
The problem exists even for guard clauses within the scripts, the script can return early within the collection level itself, and folder, request level scripts are never ran!
Fixes #6130
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.