fix(wrapDefaultExport): handle complex multiline exports #1624
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SutuSebastian
added
🐛 bug
2 tasks
🦋 Changeset detectedLatest commit: 74f92b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughRefactors wrapDefaultExport to a unified, config-driven flow handling ESM and CJS, adding robust export-value extraction, last-occurrence targeting, and class/interface skip logic. Adds tests covering complex multiline exports for both module systems. Introduces a changeset entry for a patch release noting the fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant WDE as wrapDefaultExport(content, withFunction)
participant CFG as EXPORT_CONFIGS
participant EX as extractExportValue()
participant CR as createWrappedReplacement()
Dev->>WDE: Provide file content + withFunction
WDE->>CFG: Determine module type (esm/cjs) patterns
WDE->>WDE: Find last matching default export
alt No match or default export is class/interface/abstract
WDE-->>Dev: Return original content
else Matched export value
WDE->>EX: Parse export value (handle nested (), {})
EX-->>WDE: exportValue + restContent
WDE->>CR: Build wrapped replacement
CR-->>WDE: prefix + withFunction(exportValue) + restContent
WDE-->>Dev: Return modified content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
SutuSebastian
deleted the
1623-npx-flowbite-reactlatest-init-generates-a-syntax-error-sentry-with-nextjs
branch
Merged
![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 (4)
.changeset/better-kings-fail.md (1)
5-5: Changeset looks goodPatch note is clear and scoped. Consider backticks around the function name for consistency: wrapDefaultExport →
wrapDefaultExport.-fix(wrapDefaultExport): handle complex multiline exports +fix(`wrapDefaultExport`): handle complex multiline exportspackages/ui/src/cli/utils/wrap-default-export.ts (1)
38-47: Optional: make “last occurrence” explicit and fasterDefine a lightweight
prefixRegex(/export\s+default\s+/gor/module\.exports\s*=\s*/g) to avoid the initial greedy(.*$)match. Then iteratematchAlland pick the last index. This reduces work on large files.Example config tweak (adjust main logic accordingly):
const EXPORT_CONFIGS = { esm: { skipPattern: /export\s+default\s+(?:class|interface|abstract\s+class)\s+/, - matchPattern: /(export\s+default\s+)([\s\S]*$)/m, + matchPattern: /(export\s+default\s+)([\s\S]*$)/m, + prefixRegex: /export\s+default\s+/g, }, cjs: { skipPattern: /module\.exports\s*=\s*(?:class|interface|abstract\s+class)\s+/, - matchPattern: /(module\.exports\s*=\s*)([\s\S]*$)/m, + matchPattern: /(module\.exports\s*=\s*)([\s\S]*$)/m, + prefixRegex: /module\.exports\s*=\s*/g, }, };packages/ui/src/cli/utils/wrap-default-export.test.ts (2)
109-151: Great coverage for complex multiline ESM exports; add a newline-after-default caseCurrent parser fails when a newline follows
export default. Add a test to lock this in after fixing the parser.@@ it("handles complex multiline exports with nested function calls and objects", () => { @@ }); + + it("wraps ESM default export when a newline follows `export default`", () => { + const input = ` +export default + withHOC(config, { + enabled: true + }); +`; + const expected = `export default wrapper(withHOC(config, { + enabled: true + }));`; + expect(wrapDefaultExport(input, "wrapper").trim()).toBe(expected.trim()); + });
153-193: CJS multiline test is good; add array and “last assignment wins” cases
- Add an array default export to ensure
[]depth is handled.- Add a case with multiple
module.exports =assignments to verify we wrap the last one.@@ it("handles complex multiline CJS exports with nested function calls and objects", () => { @@ }); + + it("wraps multiline array ESM default export", () => { + const input = `export default [ + 1, + 2 +];`; + const expected = `export default wrapper([ + 1, + 2 +]);`; + expect(wrapDefaultExport(input, "wrapper")).toBe(expected); + }); + + it("wraps only the last CJS assignment", () => { + const input = ` +module.exports = first; +module.exports = second; +`; + const expected = ` +module.exports = wrapper(second); +`; + expect(wrapDefaultExport(input, "wrapper").trim()).toBe(expected.trim()); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/better-kings-fail.md(1 hunks)packages/ui/src/cli/utils/wrap-default-export.test.ts(1 hunks)packages/ui/src/cli/utils/wrap-default-export.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/cli/utils/wrap-default-export.test.ts (1)
packages/ui/src/cli/utils/wrap-default-export.ts (1)
wrapDefaultExport(10-36)
Comment on lines
+21
to
33
| // Skip if it's a class/interface export | ||
| if (!content.match(config.skipPattern)) { | ||
| const lastExportMatch = wrappedContent.match(config.matchPattern); | ||
| if (lastExportMatch) { | ||
| const [fullMatch, prefix, rest] = lastExportMatch; | ||
| const { exportValue, restContent } = extractExportValue(rest); | ||
| const replacement = createWrappedReplacement(prefix, exportValue, withFunction, restContent); | ||
|
|
||
| // Replace only the last occurrence | ||
| const index = wrappedContent.lastIndexOf(fullMatch); | ||
| wrappedContent = wrappedContent.slice(0, index) + replacement + wrappedContent.slice(index + fullMatch.length); | ||
| } | ||
| } |
There was a problem hiding this comment.
"Last occurrence" logic doesn’t actually target the last export
match(config.matchPattern) captures from the first occurrence to EOF; replacing the “last occurrence” of fullMatch still uses the first prefix. This fails when multiple module.exports = (or rare multiple ESM defaults in generated code) appear; it won’t wrap the last one.
Apply this diff to locate the last prefix and compute rest from that position:
- // Skip if it's a class/interface export
- if (!content.match(config.skipPattern)) {
- const lastExportMatch = wrappedContent.match(config.matchPattern);
- if (lastExportMatch) {
- const [fullMatch, prefix, rest] = lastExportMatch;
- const { exportValue, restContent } = extractExportValue(rest);
- const replacement = createWrappedReplacement(prefix, exportValue, withFunction, restContent);
-
- // Replace only the last occurrence
- const index = wrappedContent.lastIndexOf(fullMatch);
- wrappedContent = wrappedContent.slice(0, index) + replacement + wrappedContent.slice(index + fullMatch.length);
- }
- }
+ // Skip if it's a class/interface export
+ if (!content.match(config.skipPattern)) {
+ const match = wrappedContent.match(config.matchPattern);
+ if (match) {
+ const [, prefix] = match;
+ const index = wrappedContent.lastIndexOf(prefix);
+ if (index !== -1) {
+ const restFromLast = wrappedContent.slice(index + prefix.length);
+ const { exportValue, restContent } = extractExportValue(restFromLast);
+ const replacement = createWrappedReplacement(prefix, exportValue, withFunction, restContent);
+ wrappedContent = wrappedContent.slice(0, index) + replacement;
+ }
+ }
+ }📝 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.
Suggested change
| // Skip if it's a class/interface export | |
| if (!content.match(config.skipPattern)) { | |
| const lastExportMatch = wrappedContent.match(config.matchPattern); | |
| if (lastExportMatch) { | |
| const [fullMatch, prefix, rest] = lastExportMatch; | |
| const { exportValue, restContent } = extractExportValue(rest); | |
| const replacement = createWrappedReplacement(prefix, exportValue, withFunction, restContent); | |
| // Replace only the last occurrence | |
| const index = wrappedContent.lastIndexOf(fullMatch); | |
| wrappedContent = wrappedContent.slice(0, index) + replacement + wrappedContent.slice(index + fullMatch.length); | |
| } | |
| } | |
| // Skip if it's a class/interface export | |
| if (!content.match(config.skipPattern)) { | |
| const match = wrappedContent.match(config.matchPattern); | |
| if (match) { | |
| const [, prefix] = match; | |
| const index = wrappedContent.lastIndexOf(prefix); | |
| if (index !== -1) { | |
| const restFromLast = wrappedContent.slice(index + prefix.length); | |
| const { exportValue, restContent } = extractExportValue(restFromLast); | |
| const replacement = createWrappedReplacement(prefix, exportValue, withFunction, restContent); | |
| wrappedContent = wrappedContent.slice(0, index) + replacement; | |
| } | |
| } | |
| } |
Comment on lines
+52
to
+76
| function extractExportValue(rest: string): { exportValue: string; restContent: string } { | ||
| let depth = 0; | ||
| let i = 0; | ||
|
|
||
| // Parse the export value handling nested parentheses and braces | ||
| for (i = 0; i < rest.length; i++) { | ||
| const char = rest[i]; | ||
| if (char === "(" || char === "{") depth++; | ||
| if (char === ")" || char === "}") depth--; | ||
|
|
||
| // Break on semicolon or newline if we're not inside parentheses/braces | ||
| if (depth === 0 && (char === ";" || char === "\n")) { | ||
| return { | ||
| exportValue: rest.slice(0, char === ";" ? i + 1 : i), | ||
| restContent: rest.slice(char === ";" ? i + 1 : i), | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return wrappedContent; | ||
| // If we didn't find a terminator, use the whole rest | ||
| return { | ||
| exportValue: rest, | ||
| restContent: "", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parser misses arrays and breaks when a newline follows export default
- Arrays ([]) aren’t tracked in
depth, so multiline array exports split early on the first newline. - If there’s a newline immediately after
export default(common in formatted code),exportValuebecomes empty.
Apply this diff to track brackets and skip leading whitespace/newlines:
-function extractExportValue(rest: string): { exportValue: string; restContent: string } {
- let depth = 0;
- let i = 0;
-
- // Parse the export value handling nested parentheses and braces
- for (i = 0; i < rest.length; i++) {
- const char = rest[i];
- if (char === "(" || char === "{") depth++;
- if (char === ")" || char === "}") depth--;
-
- // Break on semicolon or newline if we're not inside parentheses/braces
- if (depth === 0 && (char === ";" || char === "\n")) {
- return {
- exportValue: rest.slice(0, char === ";" ? i + 1 : i),
- restContent: rest.slice(char === ";" ? i + 1 : i),
- };
- }
- }
-
- // If we didn't find a terminator, use the whole rest
- return {
- exportValue: rest,
- restContent: "",
- };
-}
+function extractExportValue(rest: string): { exportValue: string; restContent: string } {
+ let depth = 0;
+ let i = 0;
+
+ // Skip leading whitespace/newlines between the prefix and the expression
+ while (i < rest.length && /\s/.test(rest[i])) i++;
+ const start = i;
+
+ // Parse the export value handling nested (), {}, []
+ for (; i < rest.length; i++) {
+ const char = rest[i];
+ if (char === "(" || char === "{" || char === "[") depth++;
+ else if (char === ")" || char === "}" || char === "]") depth = Math.max(0, depth - 1);
+
+ // Break on semicolon or newline if we're not inside nested structures
+ if (depth === 0 && (char === ";" || char === "\n")) {
+ return {
+ exportValue: rest.slice(start, char === ";" ? i + 1 : i),
+ restContent: rest.slice(char === ";" ? i + 1 : i),
+ };
+ }
+ }
+
+ // If we didn't find a terminator, use the remaining substring
+ return {
+ exportValue: rest.slice(start),
+ restContent: "",
+ };
+}📝 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.
Suggested change
| function extractExportValue(rest: string): { exportValue: string; restContent: string } { | |
| let depth = 0; | |
| let i = 0; | |
| // Parse the export value handling nested parentheses and braces | |
| for (i = 0; i < rest.length; i++) { | |
| const char = rest[i]; | |
| if (char === "(" || char === "{") depth++; | |
| if (char === ")" || char === "}") depth--; | |
| // Break on semicolon or newline if we're not inside parentheses/braces | |
| if (depth === 0 && (char === ";" || char === "\n")) { | |
| return { | |
| exportValue: rest.slice(0, char === ";" ? i + 1 : i), | |
| restContent: rest.slice(char === ";" ? i + 1 : i), | |
| }; | |
| } | |
| } | |
| return wrappedContent; | |
| // If we didn't find a terminator, use the whole rest | |
| return { | |
| exportValue: rest, | |
| restContent: "", | |
| }; | |
| } | |
| function extractExportValue(rest: string): { exportValue: string; restContent: string } { | |
| let depth = 0; | |
| let i = 0; | |
| // Skip leading whitespace/newlines between the prefix and the expression | |
| while (i < rest.length && /\s/.test(rest[i])) i++; | |
| const start = i; | |
| // Parse the export value handling nested (), {}, [] | |
| for (; i < rest.length; i++) { | |
| const char = rest[i]; | |
| if (char === "(" || char === "{" || char === "[") depth++; | |
| else if (char === ")" || char === "}" || char === "]") depth = Math.max(0, depth - 1); | |
| // Break on semicolon or newline if we're not inside nested structures | |
| if (depth === 0 && (char === ";" || char === "\n")) { | |
| return { | |
| exportValue: rest.slice(start, char === ";" ? i + 1 : i), | |
| restContent: rest.slice(char === ";" ? i + 1 : i), | |
| }; | |
| } | |
| } | |
| // If we didn't find a terminator, use the remaining substring | |
| return { | |
| exportValue: rest.slice(start), | |
| restContent: "", | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/ui/src/cli/utils/wrap-default-export.ts around lines 52 to 76, the
parser doesn't track square brackets and treats an immediate newline after
"export default" as a terminator, producing an empty exportValue; update the
function to skip leading whitespace/newlines before parsing and include "[" and
"]" in the depth tracking logic (increment depth for "(", "{", "[" and decrement
for ")", "}", "]") so multiline arrays and formatted exports are handled
correctly, and only treat semicolon/newline as terminators when depth is zero
after skipping leading whitespace.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit