Conversation
🦋 Changeset detectedLatest commit: 57a0d78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3073 +/- ##
==========================================
- Coverage 89.03% 89.01% -0.03%
==========================================
Files 375 370 -5
Lines 47566 46705 -861
Branches 4155 4155
==========================================
- Hits 42351 41575 -776
+ Misses 5163 5078 -85
Partials 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughCreates a centralized internal Babel façade at packages/compiler/internal/babel that re-exports Babel parsers, generators, plugins, traverse, and types. Removes in-repo Marko-specific Babel type/generator/traverse patches and instead adds patch files under patches/ for 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@babel.config.js`:
- Around line 8-10: The Babel config's targets.node is set to "20" which
prevents transpilation compatibility for the minimum supported Node version;
update the babel.config.js targets node property (the targets object, node key)
to "18" to match package.json's minimum "18 || 20 || >=22" (or alternatively
remove Node 18 from package.json engines if you intend to drop support) so Babel
emits code compatible with Node 18.
In `@package.json`:
- Line 21: The package.json uses caret ranges for Babel packages so
patch-package may skip applying version-specific patch files (e.g.,
patches/@babel+traverse+7.29.0.patch); update package.json to pin
`@babel/traverse`, `@babel/types`, and `@babel/generator` to the exact versions that
match the patch filenames (replace ^7.x ranges with the exact 7.29.0-style
versions referenced by your patches) and keep the existing "prepare" script
(prepare) and any other dependency entries in sync so patch-package will always
find and apply the correct patches.
In `@packages/compiler/internal/babel/index.ts`:
- Around line 1-22: Remove the module augmentation declaring "declare module
\"@babel/core\" { export const File: any; }" and remove "File" from the export
list so the file no longer re-exports a non-existent symbol; update any import
sites expecting File to use supported APIs (e.g., parser/transform functions or
documented helpers) or import Babel internals explicitly if intentionally
needed, and run tests to ensure no runtime module resolution errors remain.
In `@packages/compiler/package.json`:
- Around line 43-44: The package export for "./internal/babel" currently points
to TS source and the dist artifacts (dist/babel.js, dist/babel.d.ts and
dist/types/dist/traverse) are missing; update the build to produce those files
and change the export to a conditional export. Run and fix
scripts/build-babel.ts and scripts/types.js so they generate dist/babel.js and
dist/babel.d.ts (and the referenced dist/types and dist/traverse), then replace
the "./internal/babel": "./internal/babel/index.ts" export with a conditional
object that provides a "types" entry pointing to "./dist/babel.d.ts" and a
"default" (or "node" / "require" / "import" as appropriate) pointing to
"./dist/babel.js" similar to the existing "./babel-utils" export pattern; also
update babel-types.d.ts references to point at generated ./dist paths. Ensure
multiple imports from `@marko/compiler/internal/babel` (e.g., src/index.js,
src/babel-utils/parse.js, src/babel-plugin/index.js) resolve to the generated
dist artifacts.
In `@packages/compiler/scripts/types/babel-traverse.js`:
- Line 29: The import of MARKO_TYPES from "@babel/types" is invalid at runtime;
update the import in packages/compiler/scripts/types/babel-traverse.js to import
MARKO_TYPES from the module that actually exports it (or else create and export
a runtime constant named MARKO_TYPES from a local module and import that).
Locate the use of MARKO_TYPES in this file and replace the source string to the
correct module that provides the runtime export, or add a new export for
MARKO_TYPES (ensuring the export name matches) and import that instead so the
symbol is available at runtime.
In `@patches/`@babel+traverse+7.29.0.patch:
- Around line 59-61: The code registers Program parameter bindings with an
incorrect binding kind "params", so Scope analysis ignores them; update the call
to use the singular binding kind "param" instead (i.e., change
registerBinding("params", path) to registerBinding("param", path)) when checking
path.type === "Program" and path.node.params?.length to ensure parameters are
correctly registered in scope; reference the registerBinding function and the
path/node params check to locate the change.
In `@patches/`@babel+types+7.29.0.patch:
- Around line 183-191: The function markoScriptlet uses the reserved word static
as a parameter name which breaks in strict/ESM contexts; rename the parameter
(e.g., isStatic or staticFlag) in the function signature and update the returned
object to set the "static" property from that new parameter (e.g., "static":
isStatic) so the external shape remains unchanged; update any local references
to that parameter and keep exports.markoScriptlet/exports.MarkoScriptlet
pointing to the renamed function.
DylanPiercey
force-pushed
the
babel-internal-dep
branch
from
8344310 to
96dd209
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@patches/`@babel+generator+7.29.1.patch:
- Around line 288-300: The type-check in expressionCouldHaveUnenclosedWhitespace
erroneously uses the non-existent AST type "Function"; update the switch to
check the correct function node types (e.g., "FunctionExpression",
"ArrowFunctionExpression", and/or "FunctionDeclaration" as appropriate) instead
of "Function" so the condition can match real Babel AST nodes, keeping the other
cases ("AssignmentExpression", "BinaryExpression", "ConditionalExpression",
"LogicalExpression", "NewExpression") unchanged.
🧹 Nitpick comments (1)
patches/@babel+generator+7.29.1.patch (1)
368-382: Consider using nativeArray.prototype.toSpliced.The
toSplicedhelper is a polyfill, butArray.prototype.toSplicedis natively available in Node 18+ (and theenginesfield inpackages/compiler/package.jsonrequires Node 18+). You could use the native method directly for simplicity, though the current implementation is correct.
DylanPiercey
force-pushed
the
babel-internal-dep
branch
from
96dd209 to
57a0d78
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@patches/`@babel+generator+7.29.1.patch:
- Around line 342-348: The loop using while (i < bodyLen && j <
attributeTagsLen) contains a branch where the condition j < attributeTagsLen is
always true, making the final else (which pushes bodyNode and increments i)
unreachable; in the block that handles merging attributeTag and bodyNode
(variables attributeTag, bodyNode, i, j, result), remove the unreachable else
branch and simplify the logic to always push attributeTag and increment j when
location data is missing (i.e., replace the if/else chain with the single branch
that handles j < attributeTagsLen), or if the intended behavior was to allow
exhausting one side, change the loop condition to while (i < bodyLen || j <
attributeTagsLen) and keep appropriate boundary checks — update the code around
the while loop accordingly (references: i, j, bodyLen, attributeTagsLen,
attributeTag, bodyNode, result).
🧹 Nitpick comments (3)
patches/@babel+types+7.29.0.patch (1)
469-471: Consider using"Scopable"alias instead of"Scope"forMarkoTagBodyto follow Babel conventions.The
MarkoTagBodytype uses the"Scope"alias, but Babel's standard convention is to use"Scopable"for nodes that introduce a scope. Currently, manual validator patches addMarkoTagBodytoisScopable,isBlockParent, andisFunctionParent(lines 571, 579, 587). Using"Scopable"instead would eliminate these manual patches and achieve automatic validator registration, aligning with Babel's type system patterns.packages/compiler/scripts/build-babel.ts (1)
6-17: Consider surfacing esbuild warnings. Optional, but it helps catch bundle issues early.Suggested tweak
-build({ +build({ format: "cjs", bundle: true, absWorkingDir, outfile: "dist/babel.js", sourcemap: false, platform: "node", external: ["browserslist"], entryPoints: ["internal/babel/index.ts"], -}).catch((err) => { +}).then((result) => { + if (result.warnings?.length) { + console.warn(result.warnings); + process.exitCode = 1; + } +}).catch((err) => { console.error(err); process.exit(1); });patches/@babel+generator+7.29.1.patch (1)
136-136: Remove unnecessarygflag from regex.The global flag is not needed for
.test()which only returns a boolean. While harmless here since the regex is created inline, removing it improves clarity.Suggested fix
- const isMultiLine = /[\r\n]/g.test(value); + const isMultiLine = /[\r\n]/.test(value);
| + } else if (j < attributeTagsLen) { | ||
| + result.push(attributeTag); | ||
| + j++; | ||
| + } else { | ||
| + result.push(bodyNode); | ||
| + i++; | ||
| + } |
There was a problem hiding this comment.
Unreachable else branch due to loop invariant.
The condition j < attributeTagsLen at line 342 is always true within the while (i < bodyLen && j < attributeTagsLen) loop, making the else branch at lines 345-348 unreachable dead code.
If the intent is to always prioritize attributeTag when location data is missing, simplify by removing the unreachable branch:
Proposed fix
if (bodyNode.loc != null && attributeTag.loc != null) {
if (compareStartLoc(bodyNode, attributeTag) < 0) {
result.push(bodyNode);
i++;
} else {
result.push(attributeTag);
j++;
}
- } else if (j < attributeTagsLen) {
+ } else {
+ // When location data is missing, prioritize attributeTags
result.push(attributeTag);
j++;
- } else {
- result.push(bodyNode);
- i++;
}📝 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.
| + } else if (j < attributeTagsLen) { | |
| + result.push(attributeTag); | |
| + j++; | |
| + } else { | |
| + result.push(bodyNode); | |
| + i++; | |
| + } | |
| } else { | |
| // When location data is missing, prioritize attributeTags | |
| result.push(attributeTag); | |
| j++; | |
| } |
🤖 Prompt for AI Agents
In `@patches/`@babel+generator+7.29.1.patch around lines 342 - 348, The loop using
while (i < bodyLen && j < attributeTagsLen) contains a branch where the
condition j < attributeTagsLen is always true, making the final else (which
pushes bodyNode and increments i) unreachable; in the block that handles merging
attributeTag and bodyNode (variables attributeTag, bodyNode, i, j, result),
remove the unreachable else branch and simplify the logic to always push
attributeTag and increment j when location data is missing (i.e., replace the
if/else chain with the single branch that handles j < attributeTagsLen), or if
the intended behavior was to allow exhausting one side, change the loop
condition to while (i < bodyLen || j < attributeTagsLen) and keep appropriate
boundary checks — update the code around the while loop accordingly (references:
i, j, bodyLen, attributeTagsLen, attributeTag, bodyNode, result).
1 participant
Description
Removes the direct dependencies on babel modules in the @marko/compiler.
Instead the compiler now uses a pre-patched and pre-build copy of babel.
Since Marko patches some internals of babel there have been several times where a change in babel breaks Marko itself.
With this new setup we can adopt new versions of babel during our regular dependencies upgrades in the Marko project.
An added bonus is that the existing patch strategy we had for babel was execution order dependent, and also broke down when multiple copies of babel were at play. With this change that is also resolved.