diff --git a/plugins/ui5-modernization/skills/fix-cyclic-deps/SKILL.md b/plugins/ui5-modernization/skills/fix-cyclic-deps/SKILL.md index 0b686dc3..93e230e9 100644 --- a/plugins/ui5-modernization/skills/fix-cyclic-deps/SKILL.md +++ b/plugins/ui5-modernization/skills/fix-cyclic-deps/SKILL.md @@ -13,9 +13,9 @@ description: | # Fix Cyclic Module Dependencies -This skill detects and resolves cyclic module dependencies that arise during UI5 modernization. When modernization phases convert global namespace access to proper `sap.ui.define` imports, new dependency edges are added to the module graph. If these edges create a circular import (A imports B, B imports A), the UI5 AMD loader returns `undefined` for the back-edge module. +This skill detects and resolves cyclic module dependencies that arise during UI5 modernization. When modernization converts global namespace access to `sap.ui.define` imports, new dependency edges can create circular imports (A imports B, B imports A), causing the UI5 AMD loader to return `undefined` for the back-edge module. -The fix: replace the back-edge `sap.ui.define` dependency with a lazy `sap.ui.require("path/to/Module")` (synchronous form) at each call site. This retrieves the already-loaded module from the loader cache without creating a dependency edge. +The fix: replace the back-edge `sap.ui.define` dependency with a lazy `sap.ui.require("path/to/Module")` (synchronous form) at each call site, retrieving the already-loaded module from the loader cache without creating a dependency edge. ## Linter Rule @@ -23,12 +23,12 @@ The fix: replace the back-edge `sap.ui.define` dependency with a lazy `sap.ui.re |---------|-----------------|---------------------| | (none — structural) | Runtime: module is `undefined` despite correct import path | Detect cycle in dependency graph, convert back-edge to lazy `sap.ui.require()` | -This skill is NOT triggered by a UI5 linter rule. It addresses a structural problem in the module dependency graph that the linter does not check. It is triggered as the final fix phase in the modernization workflow, or standalone when a developer encounters `undefined` modules at runtime. +This skill is NOT triggered by a UI5 linter rule. It addresses a structural problem in the module dependency graph. It is triggered as the final fix phase in the modernization workflow, or standalone when a developer encounters `undefined` modules at runtime. ## When to Use -- **In modernization workflow**: As Phase 3, Step 3.3 (final step) after ALL other Phase 3 steps (3.1 globals/pseudo-modules + 3.2 blind-spots) complete. Multiple steps add `sap.ui.define` edges that can create cycles — running once at the end operates on the final dependency graph. -- **Standalone**: When a module returns `undefined` at runtime despite a correct import path in the `sap.ui.define` array. This is the classic symptom of a cyclic dependency. +- **In modernization workflow**: As Phase 3, Step 3.3 (final step) after ALL other Phase 3 steps complete. Multiple steps add `sap.ui.define` edges that can create cycles — running once at the end operates on the final dependency graph. +- **Standalone**: When a module returns `undefined` at runtime despite a correct import path. - **After manual refactoring**: When a developer adds a new `sap.ui.define` dependency and gets an unexpected `undefined`. ## Sources of Cycles @@ -41,11 +41,11 @@ Converting `var Helper = com.example.app.utils.Helper;` to a `sap.ui.define` dep ### 2. fix-js-globals Case 10 — jQuery.sap.declare/require Modernization -Wrapping legacy `jQuery.sap.declare` / `jQuery.sap.require` code in `sap.ui.define`. The `jQuery.sap.require` calls become `sap.ui.define` dependencies, potentially creating cycles that the legacy synchronous loader handled differently. +Wrapping legacy `jQuery.sap.declare` / `jQuery.sap.require` code in `sap.ui.define`. The `jQuery.sap.require` calls become dependencies, potentially creating cycles the legacy synchronous loader handled differently. ### 3. modernize-test-starter — Test File Dependencies -Test files that previously accessed modules via the global namespace chain now get proper `sap.ui.define` imports. Test utility files that reference each other, or test files that import app modules which import test utilities, can create cycles. +Test files that previously accessed modules via the global namespace chain now get proper `sap.ui.define` imports. Test utility files that reference each other can create cycles. ## Background — Why Cycles Break the UI5 Loader @@ -56,34 +56,31 @@ The UI5 AMD loader resolves `sap.ui.define` dependencies via depth-first travers 3. B depends on A → but A isn't finished yet 4. Loader returns `undefined` for A's factory result (the back-edge) 5. B's factory receives `undefined` where it expected A's exports -6. Any call to the `undefined` module causes `TypeError: Cannot read properties of undefined` -**2-node cycles (A↔B)** are guaranteed to break — the loader must pick one to load first, and the other always gets `undefined`. - -**Longer chains (A→B→C→A)** may or may not break at runtime, depending on which module the loader visits first and which edge becomes the back-edge. They are latent bugs that can surface under different load orders, lazy loading, or Test Starter isolation. +**2-node cycles (A↔B)** are guaranteed to break. **Longer chains (A→B→C→A)** may or may not break at runtime depending on load order — they are latent bugs. ### Why `sap.ui.require` (synchronous) Breaks the Cycle The synchronous form `sap.ui.require("path/to/Module")` does NOT create a loader dependency edge. It reads from the module cache without triggering a load. -**Critical caveat**: `sap.ui.require(path)` (single-string sync form) returns `undefined` if the target module's factory has not yet executed. The module's factory executes only when some static `sap.ui.define` chain has already pulled it in. So `sap.ui.require` is safe **only when the target module is reachable via a static-only path from the active entry point (Component or controller) that fires before the lazy call site**. +**Critical caveat**: `sap.ui.require(path)` returns `undefined` if the target module's factory has not yet executed. So it is safe **only when the target module is reachable via a static-only path from the active entry point** that fires before the lazy call site. -When you remove a static edge A → B and replace it with `var B = sap.ui.require("B")` inside a function in A, you must verify B remains reachable from every entry-point controller that loads A. Otherwise B stays defined-but-unevaluated in `Component-preload.js`, and the lazy require returns `undefined`. +When you remove a static edge A → B and replace it with `var B = sap.ui.require("B")` inside a function in A, you must verify B remains reachable from every entry-point controller that loads A. **Three forms of `sap.ui.require`** — know the difference: -- `sap.ui.require("path/to/B")` — sync cache read. Does NOT trigger load. Returns `undefined` if B not in cache. -- `sap.ui.require(["path/to/B"], function(B) {...})` — async load + callback. DOES trigger load. -- `sap.ui.requireSync("path/to/B")` — sync load + return. DOES trigger load. Deprecated. +- `sap.ui.require("path/to/B")` — sync cache read. No load trigger. Returns `undefined` if not in cache. +- `sap.ui.require(["path/to/B"], function(B) {...})` — async load + callback. Triggers load. +- `sap.ui.requireSync("path/to/B")` — sync load + return. Triggers load. Deprecated. **Key distinction:** -- `sap.ui.define(["path/to/B"], function(B) {...})` — creates a loader edge A→B (cycle risk) -- `var B = sap.ui.require("path/to/B")` inside a function body — no loader edge (safe, IF B is already in cache) +- `sap.ui.define(["path/to/B"], ...)` — creates a loader edge A→B (cycle risk) +- `var B = sap.ui.require("path/to/B")` inside a function body — no loader edge (safe if B is in cache) ## Detection Algorithm ### Automated Detection Script -A bundled script automates the entire detection phase (graph building, verification, cycle detection, usage counting, and hub identification). Run it first: +A bundled script automates detection (graph building, verification, cycle detection, usage counting, hub identification): ```bash node /scripts/detect-cycles.js @@ -91,196 +88,148 @@ node /scripts/detect-cycles.js The script: 1. Discovers the project namespace from `manifest.json` -2. Scans all `.js` files (app source + tests), strips comments, parses `sap.ui.define` arrays +2. Scans all `.js` files (app + tests), strips comments, parses `sap.ui.define` arrays 3. Builds the dependency graph (internal project modules only) 4. Verifies graph completeness with fallback analysis 5. Detects 2-node cycles and longer chains (Tarjan's SCC) -6. Counts usages and identifies which side gets lazy treatment (2-node) or which hub to fix (3+ node) - -Output is JSON to stdout. Use this output to drive the fix phase — no need to build the graph manually. +6. Counts usages and identifies lazy side (2-node) or hub (3+ node) -If the script is unavailable or you need to understand the algorithm details, the manual procedure is documented below and in `references/dependency-graph-analysis.md`. +Output is JSON to stdout. Use this to drive the fix phase. If unavailable, the manual procedure is below and in `references/dependency-graph-analysis.md`. ### Manual Procedure #### Step 1 — Discover Project Namespace -Read `manifest.json` to get the project namespace: - -```javascript -// manifest.json → sap.app.id → e.g. "com.example.myapp" -// Convert to slash notation: "com/example/myapp" -``` - -This namespace identifies which dependencies are internal project modules (vs. `sap/*` framework deps that never cause cycles). +Read `manifest.json` → `sap.app.id` → e.g. `"com.example.myapp"` → convert to slash notation: `"com/example/myapp"`. This identifies internal project modules vs. `sap/*` framework deps. #### Step 2 — Build Dependency Graph -Parse all `.js` files in the project (app source AND test files). For each file: +Parse all `.js` files (app source AND tests). For each file: -1. **Strip comments** before any parsing. Remove `//` single-line and `/* */` multi-line comments while preserving string contents (single-quoted, double-quoted, and template literal strings must not be corrupted). This prevents matching `sap.ui.define` inside commented-out code (e.g., `// sap.ui.define -` in a log message). +1. **Strip comments** (`//` and `/* */`) while preserving string contents. Prevents matching `sap.ui.define` in commented-out code. -2. **Find `sap.ui.define`** using a regex that allows arbitrary whitespace and newlines between all tokens: +2. **Find `sap.ui.define`** using a regex allowing arbitrary whitespace between tokens: ``` /sap\s*\.\s*ui\s*\.\s*define\s*\(/ ``` - **Critical**: The simpler regex `sap\.ui\.define\s*\(` will MISS files where `sap.ui.define` is split across lines (e.g., `sap.ui\n\t.define(` or `sap.ui.\ndefine(`). These multi-line patterns exist in real codebases and missing even one module can hide entire cycle chains. + **Critical**: The simpler `sap\.ui\.define\s*\(` MISSES files where `sap.ui.define` is split across lines. These patterns exist in real codebases. -3. **Extract the dependency array**: Starting from the `(` after `define`, skip whitespace, find `[`, then find the matching `]` respecting bracket depth. Extract all string literals from within the brackets. +3. **Extract the dependency array**: From `(` after `define`, find `[` ... `]` respecting bracket depth. Extract string literals. -4. **Filter to internal project modules** only (matching the project namespace). Framework dependencies (`sap/*`) never cause project-level cycles. Include `test-resources/`-prefixed paths — these are test module dependencies (e.g., `test-resources/com/example/myapp/test/unit/Helper`) and can participate in cycles. +4. **Filter to internal project modules** only (matching namespace). Include `test-resources/`-prefixed paths. -5. **Build a directed graph**: node = module path (relative to base dir, without `.js`), edge = dependency. For test files, the module ID uses the `test-resources/` prefix as it appears in `sap.ui.define` arrays. +5. **Build directed graph**: node = module path (without `.js`), edge = dependency. -**Important exclusions:** -- Framework dependencies (`sap/*`, `sap/ui/*`, etc.) — never cause project-level cycles -- `sap.ui.require` calls in function bodies — these are NOT dependency edges -- String literals (extend names, fragment paths, log messages) — not dependencies +**Exclusions**: Framework deps (`sap/*`), `sap.ui.require` calls in function bodies, string literals (extend names, fragment paths). #### Step 2b — Verify Graph Completeness -After building the graph, verify that every referenced dependency was also successfully parsed. A missing module can hide entire cycle chains. - -For every module referenced as a dependency in the graph that has NO entry as a graph node (0 internal deps extracted): - -1. **Check if the source file exists**. If not, it may live in a different base directory or be generated at build time — skip with an info note. - -2. **If the file exists**, run a **fallback analysis**: - - Scan the file (after comment stripping) for internal namespace strings that appear inside `[...]` array brackets - - If internal dependency strings are found inside arrays but the primary parser extracted nothing, the file likely uses an unrecognized `sap.ui.define` pattern - - Flag as ERROR and **merge the suspected dependencies into the graph** before cycle detection +After building the graph, verify every referenced dependency was also parsed. A missing module can hide entire cycle chains. -3. **Also check files where `sap.ui.define` was found but the first argument was not an array** (e.g., a string module name as first arg). Run the same fallback analysis. - -This verification step catches modules with unusual `sap.ui.define` formatting that the primary regex handles but the array extraction might miss. Without it, the graph is incomplete and cycles go undetected. +For every module referenced as a dependency but having no graph node entry: +1. Check if source file exists. If not, skip with info note. +2. If file exists, run **fallback analysis**: scan for internal namespace strings inside `[...]` arrays. If found but primary parser extracted nothing, merge into graph. +3. Also check files where `sap.ui.define` was found but first argument was not an array. Run same fallback. #### Step 3 — Detect 2-Node Cycles -For every edge A→B in the graph, check if B→A also exists. Collect unique pairs (deduplicated — A↔B and B↔A are the same cycle). +For every edge A→B, check if B→A also exists. Collect unique pairs (deduplicated). #### Step 4 — Detect Longer Chains (3+ Nodes) -Run Tarjan's strongly-connected-components (SCC) algorithm on the dependency graph. Any SCC with 3 or more nodes represents a longer cycle chain. - -See `references/dependency-graph-analysis.md` for the full algorithm pseudocode and extraction procedure. +Run Tarjan's SCC algorithm. Any SCC with 3+ nodes is a longer cycle chain. See `references/dependency-graph-analysis.md` for full algorithm pseudocode. ## Fix Strategy for 2-Node Cycles ### Decision: Which Side Gets Lazy Treatment -For each 2-node cycle A↔B, the decision has two phases: **runtime reachability** (correctness) then **usage counts** (convenience). - -#### Phase 1 — Runtime Reachability Check (NEW — prevents undefined at runtime) - -1. **Identify entry points** for the cycle pair: Component.js + every controller that statically imports A or B (directly or transitively). -2. **For each entry point**, walk the static-only dep graph and check which of {A, B} is reachable. -3. **Choose the lazy side based on coverage**: - - If A is reachable from every entry point that uses B's behavior → safe to make A lazy in B. - - If B is reachable from every entry point that uses A's behavior → safe to make B lazy in A. - - If both sides have full coverage → proceed to Phase 2 (usage counts as tiebreaker). - - If NEITHER side has full coverage from all relevant entry points → the cycle break requires ALSO adding a static dep to one or more controllers (see "Append-to-Controller Remedy" below). +#### Phase 1 — Runtime Reachability Check -#### Phase 2 — Usage Count Tiebreaker (existing logic) +1. Identify entry points: Component.js + every controller that statically imports A or B. +2. For each entry point, walk static dep graph and check which of {A, B} is reachable. +3. Choose lazy side based on coverage: + - If A reachable from every entry point using B → safe to make A lazy in B. + - If B reachable from every entry point using A → safe to make B lazy in A. + - If both have full coverage → proceed to Phase 2. + - If NEITHER has full coverage → requires also adding a static dep to controllers (see "Append-to-Controller Remedy"). -When both sides are equally safe from a reachability standpoint: +#### Phase 2 — Usage Count Tiebreaker -1. **Count usages**: How many times does A's code body reference B? How many times does B's code body reference A? (Exclude string literals, comments, and the `sap.ui.define` array itself.) -2. **Fewer usages wins**: The side with fewer usages of the other module gets the lazy treatment (fewer call sites to modify = less code churn). -3. **Tiebreaker 1**: The module with more total `sap.ui.define` dependencies keeps its normal import. It is likely an "orchestrator" module; the other is more "utility-like". -4. **Tiebreaker 2**: The module whose path sorts alphabetically first keeps its normal import. +When both sides are equally safe: +1. **Count usages**: References to B in A's code body vs. references to A in B's code body (exclude strings, comments, `sap.ui.define` array). +2. **Fewer usages wins**: Less code churn. +3. **Tiebreaker 1**: Module with more total deps keeps normal import (orchestrator pattern). +4. **Tiebreaker 2**: Alphabetically first keeps normal import. ### Transformation Steps -To make B lazy in A (i.e., A no longer statically imports B): - -**Step 1 — Remove B from A's dependency array:** +To make B lazy in A (A no longer statically imports B): +**Step 1 — Remove B from A's dependency array and corresponding parameter:** ```javascript // Before: sap.ui.define(["path/to/B", "path/to/C"], function(B, C) { - // After: sap.ui.define(["path/to/C"], function(C) { ``` -Remove the dependency string AND the corresponding function parameter at the same positional index. Verify remaining parameters still align with remaining deps. - **Step 2 — Add lazy require at each usage site:** - -At every location in A's code where B is referenced, add a `sap.ui.require` call inside the enclosing function body: - ```javascript // Before: someMethod: function() { B.doSomething(); - // ... later ... B.doSomethingElse(); } - // After: someMethod: function() { var B = sap.ui.require("path/to/B"); B.doSomething(); - // ... later ... B.doSomethingElse(); } ``` -**One `sap.ui.require` per function** — if multiple functions use B, each needs its own declaration. - -**Declaration keyword**: Match the surrounding code style. If the file uses `const`/`let`, use `const` for the lazy require (the module reference is never reassigned). If the file uses `var`, use `var`. Examples: -- `const B = sap.ui.require("path/to/B");` — modern style -- `var B = sap.ui.require("path/to/B");` — legacy style +**One `sap.ui.require` per function** — each function using B needs its own declaration. -**Step 3 — Clean up artifacts:** +**Declaration keyword**: Match surrounding style (`const` for modern, `var` for legacy). -- If removing B's dep leaves a `var B = B;` self-assignment, delete that line -- If B was imported but had zero code references (unused dep), just remove from deps and params — no lazy require needed +**Step 3 — Clean up**: Remove `var X = X;` self-assignments. If B had zero code references, just remove from deps — no lazy require needed. ### Special Case: Module Already Partially Lazy -If A already has some `sap.ui.require("path/to/B")` calls (from a previous partial fix), and ALSO has B in its `sap.ui.define` array, remove B from `sap.ui.define` and ensure all remaining usage sites have lazy requires. Don't duplicate existing lazy require calls. +If A already has `sap.ui.require("path/to/B")` calls AND B in its `sap.ui.define` array, remove B from `sap.ui.define` and ensure all remaining usage sites have lazy requires. Don't duplicate existing calls. ## Longer Chains (3+ Nodes) — Hub-Based Auto-Fix -Longer chains are also auto-fixed using a **hub-based approach**. Instead of tracing individual cycle paths, identify the hub module within each strongly connected component (SCC) and make its cycle-creating dependencies lazy. One hub fix can eliminate many chains simultaneously. +Longer chains are auto-fixed using a **hub-based approach**. Identify the hub module within each SCC and make its cycle-creating dependencies lazy. One hub fix can eliminate many chains simultaneously. ### Why Hub-Based? -A hub module sits at the center of multiple cycle paths. For example, a `ModelManager` might participate in 7 different 3–5 node cycles. Rather than fixing 7 edges in 7 files, converting 2 deps in `ModelManager` to lazy requires breaks all 7 cycles at once. +A hub module sits at the center of multiple cycle paths. Rather than fixing edges in many files, converting a few deps in the hub breaks all cycles at once. ### Hub Identification Algorithm For each SCC with 3+ nodes: -1. **Compute cycle participation score** for each node in the SCC: - - Count how many of the node's `sap.ui.define` deps are also in the SCC (outgoing SCC edges) - - Score = outgoing SCC edge count (more outgoing edges = more cycles broken by making those deps lazy) - -2. **Select the hub**: The node with the highest score. Tiebreaker: the node with more total `sap.ui.define` dependencies (it is the orchestrator module). Final tiebreaker: alphabetical by module path. - -3. **Identify cycle-creating deps**: From the hub's `sap.ui.define` array, find which deps are members of the same SCC. These are the deps that participate in cycles. - -4. **Apply the same transformation as 2-node cycles**: Remove each cycle-creating dep from the hub's `sap.ui.define` array, add lazy `sap.ui.require` at each usage site. - -5. **Re-run SCC detection**: If cycles remain (possible when an SCC has multiple independent hubs), repeat from step 1 on the remaining SCCs. +1. **Cycle participation score** per node: count outgoing edges to other SCC members. Higher = more cycles broken by making those deps lazy. +2. **Select hub**: Highest score. Tiebreaker: more total deps (orchestrator). Final: alphabetical. +3. **Identify cycle-creating deps**: Hub's `sap.ui.define` deps that are SCC members. +4. **Apply same transformation as 2-node cycles**. +5. **Re-run SCC detection**: Repeat if cycles remain. ### Decision: Which Hub Deps to Make Lazy -Not all of a hub's deps within the SCC need to be made lazy — only those that create back-paths. However, in practice it is safest to **make all SCC-internal deps of the hub lazy**. This guarantees all cycles through the hub are broken, and the cost is minimal (lazy `sap.ui.require` at a few call sites). - -If a hub's SCC-internal dep has zero code usages (unused import), just remove it — no lazy require needed. +Make **all SCC-internal deps of the hub lazy**. This guarantees all cycles through the hub are broken. If a hub's SCC-internal dep has zero usages, just remove it. ### Fallback: Report to MODERNIZATION-ISSUES.md -If the hub-based fix cannot be applied (e.g., a module references the cycle-creating dep at the top level outside any function body, where lazy `sap.ui.require` would return `undefined`), report the chain in `MODERNIZATION-ISSUES.md`: +If a module references the cycle-creating dep at top level outside any function body (where lazy `sap.ui.require` would return `undefined`), report: ```markdown ### Cyclic Dependency Chain (unfixable automatically) - **SCC nodes**: A, B, C, D - **Hub identified**: A (3 outgoing SCC edges, 4 incoming) -- **Blocking reason**: A references B at module top level (line 15), outside any function body. Lazy `sap.ui.require` at this location may return `undefined` because B is not yet loaded during A's module initialization. +- **Blocking reason**: A references B at module top level (line 15), outside any function body. - **Suggested manual fix**: Restructure A to defer the B reference into a function body, or extract the top-level initialization into a separate non-cyclic module. ``` @@ -290,7 +239,7 @@ If the hub-based fix cannot be applied (e.g., a module references the cycle-crea ModuleA imports ModuleB, ModuleB imports ModuleA. ModuleB only uses ModuleA at 1 call site. -**Before (broken — cycle causes ModuleA to be `undefined` in ModuleB):** +**Before (broken):** ```javascript // ModuleB.js sap.ui.define([ @@ -299,14 +248,14 @@ sap.ui.define([ ], function(ModuleA, jQuery) { var ModuleB = { handleStatus: function(aSelectedItems, sId) { - ModuleA.processStatus(aSelectedItems, sId); // ModuleA is undefined! + ModuleA.processStatus(aSelectedItems, sId); // undefined! } }; return ModuleB; }); ``` -**After (fixed — lazy require at call site):** +**After (fixed):** ```javascript // ModuleB.js sap.ui.define([ @@ -315,7 +264,7 @@ sap.ui.define([ var ModuleB = { handleStatus: function(aSelectedItems, sId) { var ModuleA = sap.ui.require("com/example/myapp/utils/ModuleA"); - ModuleA.processStatus(aSelectedItems, sId); // Works: ModuleA loaded from cache + ModuleA.processStatus(aSelectedItems, sId); } }; return ModuleB; @@ -331,16 +280,11 @@ Orchestrator imports Helper (5 usages), Helper imports Orchestrator (25 usages). // Orchestrator.js sap.ui.define([ "com/example/myapp/utils/Helper", - "com/example/myapp/utils/Validator", - // ... other deps -], function(Helper, Validator, ...) { + "com/example/myapp/utils/Validator" +], function(Helper, Validator) { var Orchestrator = { - openDialog: function() { - Helper.openDialog(); // Helper could be undefined - }, - refreshAll: function() { - Helper.refreshAll(); - } + openDialog: function() { Helper.openDialog(); }, + refreshAll: function() { Helper.refreshAll(); } // ... 3 more Helper usage sites }; return Orchestrator; @@ -351,9 +295,8 @@ sap.ui.define([ ```javascript // Orchestrator.js sap.ui.define([ - "com/example/myapp/utils/Validator", - // ... other deps (Helper removed) -], function(Validator, ...) { + "com/example/myapp/utils/Validator" +], function(Validator) { var Orchestrator = { openDialog: function() { var Helper = sap.ui.require("com/example/myapp/utils/Helper"); @@ -369,23 +312,21 @@ sap.ui.define([ }); ``` -### Example 3 — Unused Dependency Removal (ModuleX → ModuleY) +### Example 3 — Unused Dependency Removal -ModuleX imports ModuleY but never references it in code. No cycle fix needed — just remove the dead import. +ModuleX imports ModuleY but never references it in code — just remove the dead import. **Before:** ```javascript -// ModuleX.js sap.ui.define([ "com/example/myapp/utils/ModuleY", "com/example/myapp/utils/ModuleZ" ], function(ModuleY, ModuleZ) { - // ModuleY never used anywhere in the file body + // ModuleY never used ``` **After:** ```javascript -// ModuleX.js sap.ui.define([ "com/example/myapp/utils/ModuleZ" ], function(ModuleZ) { @@ -393,15 +334,9 @@ sap.ui.define([ ### Example 4 — Longer Chain Auto-Fixed via Hub (ModelManager Hub) -ModelManager participates in 7 cycle chains through its deps FilterHelper and ChartHelper: -- ModelManager → FilterHelper → Formatter → ModelManager -- ModelManager → FilterHelper → MessageHelper → ModelManager -- ModelManager → ChartHelper → ReportHelper → ModelManager -- ... and 4 more chains +ModelManager participates in 7 cycle chains through deps FilterHelper and ChartHelper. Hub analysis: ModelManager has 2 outgoing SCC edges — it is the hub. -**Hub analysis**: ModelManager has 2 outgoing SCC edges (FilterHelper, ChartHelper) and multiple incoming edges. It is the hub. - -**Before (broken — ModelManager is `undefined` for downstream importers):** +**Before (broken):** ```javascript // ModelManager.js sap.ui.define(["sap/ui/thirdparty/jquery", @@ -409,10 +344,8 @@ sap.ui.define(["sap/ui/thirdparty/jquery", "com/example/myapp/utils/Payload", "com/example/myapp/utils/ChartHelper", "sap/base/Log" -], -function (jQuery, FilterHelper, oPayload, ChartHelper, Log) { +], function(jQuery, FilterHelper, oPayload, ChartHelper, Log) { var ModelManager = { - // ... 1 usage of FilterHelper, 2 usages of ChartHelper getFilterConfig: function() { var oFilterConfig = FilterHelper; // undefined due to cycle! }, @@ -430,8 +363,7 @@ function (jQuery, FilterHelper, oPayload, ChartHelper, Log) { sap.ui.define(["sap/ui/thirdparty/jquery", "com/example/myapp/utils/Payload", "sap/base/Log" -], -function (jQuery, oPayload, Log) { +], function(jQuery, oPayload, Log) { var ModelManager = { getFilterConfig: function() { var FilterHelper = sap.ui.require("com/example/myapp/utils/FilterHelper"); @@ -446,102 +378,99 @@ function (jQuery, oPayload, Log) { }); ``` -**Result**: Removing 2 deps from 1 file broke all 7 cycle chains simultaneously. Note: `var oFilterConfig = FilterHelper;` is a common legacy pattern (aliasing the module reference). The skill preserves existing code patterns — it only transforms the import mechanism, not the surrounding code. +Removing 2 deps from 1 file broke all 7 cycle chains. Note: `var oFilterConfig = FilterHelper;` is a legacy aliasing pattern — the skill preserves existing code, only transforming the import mechanism. ## Implementation Steps -1. **Run the detection script** to build the dependency graph and detect all cycles: +1. **Run detection script** to build the dependency graph and detect all cycles: ```bash node /scripts/detect-cycles.js ``` - The JSON output contains `twoNodeCycles` (with usage counts and lazy-side decisions) and `longerChains` (with hub identification). If the script is unavailable, follow the manual procedure in the Detection Algorithm section. + JSON output contains `twoNodeCycles` (with usage counts and lazy-side decisions) and `longerChains` (with hub identification). If unavailable, follow the manual procedure above. -2. **Review the script output**: Check `warnings` and `errors` arrays for graph completeness issues. Address any errors before proceeding. +2. **Review output**: Check `warnings` and `errors` arrays for graph completeness issues. Address errors before proceeding. 3. **Fix each 2-node cycle** (from `twoNodeCycles[]`): - - The script's `lazySide` field tells you which module to patch - - Remove the cyclic dep from that module's `sap.ui.define` array + function parameter - - Add `var Module = sap.ui.require("path/to/Module")` at each usage site within function bodies - - Clean up artifacts (`var X = X;` lines, unused deps) - -4. **Fix longer chains via hub approach** (from `longerChains[]`): - - The script's `hub` and `hubInternalDeps` fields identify which module and deps to make lazy - - For each hub-internal dep: check all usage sites in the hub's code — if ALL usages are inside function bodies, apply lazy require transformation (same as step 3) - - If any usage is at module top level (outside a function body), report to MODERNIZATION-ISSUES.md instead - - Re-run SCC detection on the updated graph — repeat if cycles remain - -5. **Verify — re-run cycle detection**: Run the detection script again on the modified codebase to confirm zero cycles remain: + - `lazySide` field identifies which module to patch + - Remove cyclic dep from `sap.ui.define` array + function parameter + - Add `var Module = sap.ui.require("path/to/Module")` at each usage site + - Clean up artifacts + +4. **Fix longer chains via hub** (from `longerChains[]`): + - `hub` and `hubInternalDeps` fields identify module and deps to make lazy + - If ALL usages are inside function bodies → apply lazy require transformation + - If any usage at module top level → report to MODERNIZATION-ISSUES.md + - Re-run SCC detection — repeat if cycles remain + +5. **Verify — re-run cycle detection**: ```bash node /scripts/detect-cycles.js ``` - The output should show `twoNodeCycles: []` and `longerChains: []`. If cycles persist, return to step 3/4. Then run `npx @ui5/linter --details` as a secondary check for regressions in other linter rules. + Output should show `twoNodeCycles: []` and `longerChains: []`. Then run `npx @ui5/linter --details` for regression check. -6. **Verify static coverage — detect unsafe lazy requires**: After cycles are cleared, run the static-coverage post-check: +6. **Verify static coverage**: ```bash node /scripts/detect-unsafe-lazy.js ``` - This script checks that every `sap.ui.require("M")` call in the codebase has its target M reachable via a static-only `sap.ui.define` chain from every entry-point controller that loads the calling file. If any findings exist, apply the "Append-to-Controller Remedy" below. Re-run until output shows `unsafeCount: 0`. **The skill cannot mark "done" until BOTH `detect-cycles.js` reports 0 cycles AND `detect-unsafe-lazy.js` reports 0 unsafe lazy requires.** + Checks every `sap.ui.require("M")` has M reachable via static chain from every entry-point controller. If findings exist, apply "Append-to-Controller Remedy" below. Re-run until `unsafeCount: 0`. **Skill is not done until BOTH scripts report 0 issues.** ### Append-to-Controller Remedy -When `detect-unsafe-lazy.js` reports uncovered entry points, the fix is to append the lazy target to the `sap.ui.define` dep array of each uncovered controller. This is a load-only side-effect import — no factory parameter needed if the controller doesn't reference the module directly: +When `detect-unsafe-lazy.js` reports uncovered entry points, append the lazy target to the controller's `sap.ui.define` array as a load-only side-effect import: ```javascript -// Before — Detail.controller.js does NOT statically import DialogHelper +// Before — controller does NOT statically import DialogHelper sap.ui.define([ "com/example/myapp/utils/ActionHandler", "sap/ui/core/mvc/Controller" ], function(ActionHandler, Controller) { // ActionHandler has lazy require to DialogHelper - // DialogHelper is NOT in cache when this controller's route activates - // → sap.ui.require("DialogHelper") returns undefined → crash + // → sap.ui.require("DialogHelper") returns undefined -// After — append DialogHelper as load-only dep +// After — append as load-only dep (no factory param) sap.ui.define([ "com/example/myapp/utils/ActionHandler", "sap/ui/core/mvc/Controller", - "com/example/myapp/utils/DialogHelper" // load-only, no factory param + "com/example/myapp/utils/DialogHelper" ], function(ActionHandler, Controller /* no DialogHelper param */) { - // DialogHelper is now evaluated before any function in ActionHandler fires - // → sap.ui.require("DialogHelper") returns the module object ✓ + // DialogHelper now in cache → lazy require works ``` -**Rules for the append-to-controller fix:** -- Append at end of dep array -- No corresponding factory parameter (unless the controller also uses the module directly) -- If `staticUncovered` lists more than 3 controllers, consider adding the dep to `BaseController.js` or `Component.js` instead (single patch, broader coverage) -- Re-run `detect-unsafe-lazy.js` after each fix to confirm the finding is resolved +**Rules:** +- Append at end of dep array, no corresponding factory parameter +- If `staticUncovered` lists 3+ controllers, add dep to `BaseController.js` or `Component.js` instead +- Re-run `detect-unsafe-lazy.js` after each fix to confirm resolution ## Notes — Critical Rules -1. **Lazy require INSIDE function body**: Place `var B = sap.ui.require("path/to/B")` inside the function that uses B, not at module top level. At define-time, B may not be in the cache yet. At function-call-time (runtime), B is in the cache **only if** some static path from the active entry point has already loaded it. This is why the static-coverage check (step 6) is essential. +1. **Lazy require INSIDE function body**: Place `var B = sap.ui.require("path/to/B")` inside the function that uses B, not at module top level. At define-time, B may not be in cache yet. -2. **One require per function**: Each function body that uses the lazy-required module needs its own `var B = sap.ui.require(...)` declaration. The variable is function-scoped — it cannot be shared across functions. +2. **One require per function**: Each function body using the module needs its own declaration. The variable is function-scoped. -3. **Only internal project modules**: Framework dependencies (`sap/*`) never cause project-level cycles. Only process dependencies whose path contains the project namespace. +3. **Only internal project modules**: Framework deps (`sap/*`) never cause project-level cycles. Only process deps matching the project namespace. -4. **String literals are NOT usages**: Log messages (`Log.error("path/to/Module/method")`), `.extend()` class names, fragment paths, and `sFileName`/`sMethodName` assignments are strings, not code references. Do not count them as usages. Do not modify them. +4. **String literals are NOT usages**: Log messages, `.extend()` class names, fragment paths are strings, not code references. Do not count or modify them. -5. **Commented-out code is NOT a usage**: Lines inside `//` or `/* */` blocks do not count as usages and should not receive lazy require treatment. +5. **Commented-out code is NOT a usage**: Lines inside `//` or `/* */` do not count as usages. -6. **Parameter alignment**: When removing a dependency at index N in the array, remove the function parameter at the same positional index N. If there are fewer parameters than dependencies (trailing side-effect imports without params), only adjust if the removed dep had a corresponding parameter. +6. **Parameter alignment**: Removing dep at index N → remove function parameter at index N. Adjust only if the removed dep had a corresponding parameter. -7. **Existing lazy requires are NOT dependency edges**: If a file already contains `sap.ui.require("path/to/X")` calls in function bodies, those do NOT create loader dependency edges. Only `sap.ui.define` array entries are edges. Do not double-count. +7. **Existing lazy requires are NOT dependency edges**: `sap.ui.require("path/to/X")` in function bodies does NOT create loader edges. Only `sap.ui.define` array entries are edges. -8. **Idempotent**: Running this skill twice on the same codebase is safe. On the second run, the cycles are already broken (the back-edge was removed from `sap.ui.define`), so no changes are made. +8. **Idempotent**: Running twice is safe. On second run, cycles are already broken — no changes made. -9. **Test files included**: The dependency graph must include test files (`test/unit/`, `test/integration/`, `test/opa/`). Test files can participate in cycles, especially after `modernize-test-starter` adds `sap.ui.define` dependencies. +9. **Test files included**: Dependency graph must include test files. They can participate in cycles after `modernize-test-starter` adds `sap.ui.define` deps. -10. **Do not touch `sap.ui.require` async callbacks**: The async form `sap.ui.require(["path/to/B"], function(B) {...})` is a different pattern and creates a loader edge. This skill only uses the synchronous single-string form. +10. **Do not touch async `sap.ui.require`**: The async form `sap.ui.require(["path/to/B"], function(B) {...})` creates a loader edge. This skill only uses the synchronous single-string form. -11. **Multi-line `sap.ui.define` patterns**: Real codebases contain `sap.ui.define` split across lines (e.g., `sap.ui\n\t.define(`, `sap.ui.\ndefine(`). The regex MUST allow arbitrary whitespace/newlines between `sap`, `.`, `ui`, `.`, `define`, and `(`. Using a simpler regex like `sap\.ui\.define\s*\(` will miss these files entirely. Missing one module can hide an entire cycle chain. +11. **Multi-line `sap.ui.define` patterns**: Regex MUST allow arbitrary whitespace/newlines between `sap`, `.`, `ui`, `.`, `define`, `(`. Simpler regexes miss split patterns, hiding cycle chains. -12. **Strip comments before parsing**: Always remove `//` and `/* */` comments (respecting string literals) before searching for `sap.ui.define`. Files may contain `sap.ui.define` in comments (e.g., `// sap.ui.define -` as a section marker) that would produce false matches. +12. **Strip comments before parsing**: Always remove comments (respecting string literals) before searching for `sap.ui.define`. Files may contain it in comments as section markers. -13. **Verify graph completeness**: After building the graph, check that every referenced dependency was also parsed. Run fallback analysis on files where the primary parser found nothing — they may use unrecognized `sap.ui.define` patterns. Merge any discovered dependencies into the graph before cycle detection. +13. **Verify graph completeness**: Check every referenced dependency was parsed. Run fallback analysis on files where primary parser found nothing. Merge discovered deps before cycle detection. ## Related Skills -- **fix-js-globals**: Case 1c (global namespace reads → imports) and case 10 (jQuery.sap.declare wrapping) are the primary sources of new dependency edges that create cycles. Run `fix-cyclic-deps` AFTER `fix-js-globals` completes. +- **fix-js-globals**: Case 1c and case 10 are primary sources of new dependency edges creating cycles. Run `fix-cyclic-deps` AFTER `fix-js-globals` completes. - **modernize-test-starter**: Test file modernization adds `sap.ui.define` dependencies that can create cycles between test utilities or between test and app modules. -- **modernize-ui5-app**: Parent workflow that orchestrates phase ordering. This skill runs as Phase 3, Step 3.3 (final step) after all other Phase 3 steps complete. +- **modernize-ui5-app**: Parent workflow that orchestrates phase ordering. This skill runs as Phase 3, Step 3.3 (final step). diff --git a/plugins/ui5-modernization/skills/fix-js-globals/SKILL.md b/plugins/ui5-modernization/skills/fix-js-globals/SKILL.md index fc11f0fd..73327a6e 100644 --- a/plugins/ui5-modernization/skills/fix-js-globals/SKILL.md +++ b/plugins/ui5-modernization/skills/fix-js-globals/SKILL.md @@ -20,68 +20,19 @@ description: | # Fix JavaScript Global Access (no-globals) -This skill fixes `no-globals` errors in JavaScript files that the UI5 linter detects but cannot auto-fix. The linter's auto-fix only works for simple read-access patterns; this skill handles the complex cases. - -## Linter Rule - -| Rule ID | Message Pattern | -|---------|-----------------| -| `no-globals` | Access of global variable '...' (...) | - -## Getting More Information with --details - -Run the linter with the `--details` flag to get additional information about deprecated APIs and their replacements: - -```bash -npx @ui5/linter --details -``` - -This provides: -- Links to API documentation -- Suggested replacement modules -- Modernization guidance for deprecated APIs - -Example output with --details: -``` -App.controller.js:5:1 error Access of global variable 'getCore' (sap.ui.getCore) no-globals - Details: Do not use global variables to access UI5 modules or APIs. - See: https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712 -``` - -## When Autofix Works vs When It Doesn't - -### Linter CAN Auto-fix -- Simple property access: `sap.m.Button` → adds to `sap.ui.define` dependencies -- Method calls on known modules: `sap.ui.require(...)` (allowed) - -### Linter CANNOT Auto-fix (This Skill Handles) -1. **Assignments to globals**: `sap.myNamespace = {...}` -1b. **Global namespace assignment inside sap.ui.define**: `my.app.Module = {...}; return my.app.Module;` — already in AMD module but still using global namespace for assignment/return -1c. **Global namespace read-only reference inside sap.ui.define**: `var x = com.example.app.utils.Helper` — already in AMD module but reads from a global namespace instead of importing it as a dependency -2. **Delete expressions**: `delete sap.something` -3. **sap.ui.core.Core access**: Global provides class, module provides singleton -4. **jQuery/$ globals**: `jQuery(...)`, `$(".selector")`, `jQuery.each()`, `jQuery.extend()`, `jQuery.proxy()` — add `sap/ui/thirdparty/jquery` dependency, replace `$` with dependency variable, keep all jQuery API calls unchanged -4b. **jQuery.sap.* utilities**: `jQuery.sap.log`, `jQuery.sap.uid`, etc. — replace with dedicated modules -5. **Conditional/probing access**: `if (sap.ui.something)` -6. **Custom namespace definitions**: Non-UI5 module namespaces -7. **Binding type strings**: `type: "sap.ui.model.type.Integer"` without import -8. **sap.ui.getCore() calls**: Needs special transformation -9. **sap.ui.controller() factory**: Controller definition or instantiation via deprecated global -10. **jQuery.sap.declare/require**: Legacy module definitions without sap.ui.define wrapper -11. **Runtime globals as module imports**: `sap.ushell.Container` and other runtime-provided modules accessed via global namespace chains — add as `sap.ui.define` dependency -12. **Sync XHR guards**: After modernizing `jQuery.sap.sjax` to native `XMLHttpRequest`, guard `xhr.responseText` with `readyState === 4 && status === 200` +Fixes `no-globals` errors in JavaScript files that the UI5 linter detects but cannot auto-fix. Run `npx @ui5/linter --details` to get replacement suggestions and documentation links. ## Key Rules — Read Before Applying Any Fix 1. **jQuery/$ globals — preserve jQuery API calls**: When fixing jQuery/$ globals, ONLY add the `sap/ui/thirdparty/jquery` dependency and replace `$` with `jQuery`. Do NOT replace standard jQuery API calls (`jQuery.each`, `jQuery.extend`, `jQuery.proxy`, `jQuery.isEmptyObject`, etc.) with native JavaScript equivalents. These are standard jQuery methods, not deprecated SAP APIs. -2. **Case 9/10 — fix ALL globals in a single pass**: When converting a file from `jQuery.sap.declare`/`require` to `sap.ui.define` (Case 9 or 10), you MUST also fix ALL other global-access patterns inside the file body in the same pass. After the structural conversion, scan the entire file for: `jQuery("#prefix--id").control(0)` → `this.byId("id")`, `sap.ui.getCore().byId(...)` → `this.byId(...)`, `jQuery.sap.*` utilities → dedicated modules, inline class references like `new sap.ui.model.json.JSONModel()` → imported dependency. There is no second pass — everything must be handled at once. Read the "Apply ALL Applicable Cases in a Single Pass" section below. +2. **Case 9/10 — fix ALL globals in a single pass**: When converting a file from `jQuery.sap.declare`/`require` to `sap.ui.define` (Case 9 or 10), you MUST also fix ALL other global-access patterns inside the file body in the same pass. There is no second pass — everything must be handled at once. Read the "Apply ALL Applicable Cases in a Single Pass" section below. -3. **Dead code — delete, don't import**: If a global assignment stores a value that is never read anywhere else in the file (e.g., `this.BarColor = sap.ui.core.BarColor` where `this.BarColor` never appears again), delete the entire statement. Do NOT add an import for it — that would introduce an unused dependency. +3. **Dead code — delete, don't import**: If a global assignment stores a value that is never read anywhere else in the file (e.g., `this.BarColor = sap.ui.core.BarColor` where `this.BarColor` never appears again), delete the entire statement. Do NOT add an import for it. -4. **No intermediate forms for byId in controllers**: `sap.ui.getCore().byId("prefix--id")` or `jQuery("#prefix--id").control(0)` inside a controller → `this.byId("id")` directly. Never leave it as `Element.getElementById("prefix--id")` — if the ID contains `--` and you're inside the owning controller, collapse it to `this.byId()` in one step. After replacing, if `Element` or `jQuery` are no longer used elsewhere in the file, remove them from the dependency array. +4. **No intermediate forms for byId in controllers**: `sap.ui.getCore().byId("prefix--id")` or `jQuery("#prefix--id").control(0)` inside a controller → `this.byId("id")` directly. Never leave it as `Element.getElementById("prefix--id")`. After replacing, remove unused `Element` or `jQuery` imports. -5. **merge, not deepExtend**: `jQuery.sap.extend(true, ...)` → `merge()` from `sap/base/util/merge`. The module `sap/base/util/deepExtend` does NOT exist — importing it will cause a runtime error. The module is called `merge` and it performs deep copy. +5. **merge, not deepExtend**: `jQuery.sap.extend(true, ...)` → `merge()` from `sap/base/util/merge`. The module `sap/base/util/deepExtend` does NOT exist. ## Fix Strategies by Case @@ -90,915 +41,373 @@ App.controller.js:5:1 error Access of global variable 'getCore' (sap.ui.getCore) **Problem**: Code creates custom namespaces on the global `sap` object. ```javascript -// Before - CANNOT be auto-fixed +// Before sap.ui.demo = sap.ui.demo || {}; -sap.ui.demo.myApp = { - formatter: function() { ... } -}; -``` +sap.ui.demo.myApp = { formatter: function() { ... } }; -**Fix Strategy**: Convert to proper AMD module definition. - -```javascript -// After - myApp.js +// After — convert to AMD module sap.ui.define("sap/ui/demo/myApp", [], function() { "use strict"; - return { - formatter: function() { ... } - }; + return { formatter: function() { ... } }; }); ``` -### 1b. Global Namespace Usage Inside sap.ui.define / sap.ui.require +### 1b. Global Namespace Assignment Inside sap.ui.define -**Problem**: File is already wrapped in `sap.ui.define` but still assigns to a deeply-nested global namespace and returns the global reference. This is a leftover from modernization where `jQuery.sap.declare` was removed but the global assignment pattern was not cleaned up. +**Problem**: File is already in `sap.ui.define` but still assigns to a global namespace and returns the global reference (leftover from `jQuery.sap.declare` removal). -**Important — not reported by linter**: The UI5 linter does NOT flag this pattern as a `no-globals` error because the global namespace path (e.g., `com.example.app.test.utils.MyTestScripts`) is not in the `sap.*` namespace that the linter checks. You must **actively search** for this pattern using grep: - -```bash -# Search for global namespace assignments inside sap.ui.define modules -grep -rl "your\.project\.namespace\." webapp/ --include="*.js" -``` - -Look for lines matching: `.. = {` and `return ..;` +**Not reported by linter** — search manually: `grep -rl "your\.project\.namespace\." webapp/ --include="*.js"` ```javascript -// Before - CANNOT be auto-fixed +// Before sap.ui.define([], function() { "use strict"; - com.example.app.test.utils.MyTestScripts = { - runTests: function(Given, When, Then) { ... }, - checkSomething: function() { ... } - }; - return com.example.app.test.utils.MyTestScripts; + com.example.app.utils.MyScripts = { runTests: function() { ... } }; + return com.example.app.utils.MyScripts; }); -``` - -**Fix Strategy**: Replace the global namespace assignment with a local variable and return that instead. The module path in `sap.ui.define` already ensures this file is loadable by its namespace — the global assignment is redundant. -```javascript -// After +// After — local variable replaces global assignment sap.ui.define([], function() { "use strict"; - var MyTestScripts = { - runTests: function(Given, When, Then) { ... }, - checkSomething: function() { ... } - }; - return MyTestScripts; + var MyScripts = { runTests: function() { ... } }; + return MyScripts; }); ``` -**Key rules:** -- Extract the short name from the end of the namespace (e.g., `com.example.app.test.utils.MyTestScripts` → `MyTestScripts`) -- Replace the assignment target with `var ShortName` — the `var` keyword is essential to scope the variable locally; omitting it would create an implicit global -- Replace the `return full.namespace.path.ShortName;` with `return ShortName;` -- Also replace any other references to the full namespace path within the same file with the local variable name -- This applies to any custom namespace, not just `sap.*` +**Key rules**: Extract short name from namespace end. Use `var ShortName` (scoping is essential). Replace all references to the full namespace within the file. ### 1c. Global Namespace Read-Only Reference Inside sap.ui.define -**Problem**: File is already wrapped in `sap.ui.define` but reads from a global namespace via a local variable assignment instead of importing the module as a dependency. The referenced module exists as a proper AMD module elsewhere — but this consumer file still accesses it through the legacy global namespace. - -This is a different pattern from 1b: there is **no assignment to** the global namespace, only a **read from** it. The file does not define the module — it consumes it. +**Problem**: File reads from a global namespace via variable assignment instead of importing the module as a dependency. -**Important — not reported by linter**: Like 1b, the UI5 linter does NOT flag this pattern when the namespace is outside `sap.*`. Search for it with grep: - -```bash -# Search for read-only global namespace references inside sap.ui.define modules -grep -rn "var .* = your\.project\.namespace\." webapp/ --include="*.js" -``` - -Look for lines matching: `var = ..;` +**Not reported by linter** — search: `grep -rn "var .* = your\.project\.namespace\." webapp/ --include="*.js"` ```javascript -// Before - CANNOT be auto-fixed -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - "use strict"; - +// Before +sap.ui.define(["sap/ui/core/mvc/Controller"], function(Controller) { var Helper = com.example.app.utils.Helper; - - return Controller.extend("com.example.app.controller.Main", { - onInit: function() { - Helper.doSomething(); - } - }); + // ... }); -``` - -**Fix Strategy**: Add the referenced module as a dependency to `sap.ui.define` and remove the local variable assignment. The dependency parameter replaces the local variable — since they typically share the same name, all downstream usage works without further changes. -```javascript -// After +// After — add as dependency, remove local var assignment sap.ui.define([ "com/example/app/utils/Helper", "sap/ui/core/mvc/Controller" ], function(Helper, Controller) { - "use strict"; - - return Controller.extend("com.example.app.controller.Main", { - onInit: function() { - Helper.doSomething(); - } - }); + // Helper is now available via dependency parameter }); ``` **Key rules:** -- Convert the dot-notation namespace to a slash-notation module path: `com.example.app.utils.Helper` → `"com/example/app/utils/Helper"` -- Add the module path to the `sap.ui.define` dependency array (at the **beginning**, per the dependency insertion rule in the Notes section) -- Add a corresponding parameter name to the factory function (matching the local variable name that was used) -- Remove the `var = ;` line — the dependency parameter now serves the same purpose -- If the file has **multiple** read-only global namespace references, add all of them as dependencies in a single pass -- **Parameter name verification**: Before replacing a global reference, verify that the `sap.ui.define` function parameter name matches the module's short name. If the module is already a dependency but the parameter has a different name (e.g., `function(Formatter)` for a `DataService` module), rename the parameter to match the module name first, then replace global references. Delete any resulting `var X = X;` self-assignment lines. -- **Atomicity — never rename without an import**: Every global→local replacement MUST be paired with a `sap.ui.define` dependency. A renamed variable without a declaration causes `ReferenceError` — worse than leaving the global. Do NOT attempt to detect cycles at this stage — cycles introduced here will be detected and resolved by `fix-cyclic-deps` in Phase 3, Step 3.3 after all fix steps complete. -- **Post-fix validation**: After fixing a file, grep for every introduced variable name and confirm it resolves to a function parameter, `var`/`let`/`const` declaration, or `sap.ui.require` call. Well-known globals that don't need imports: `window`, `document`, `console`, `sap`, `jQuery`, `QUnit`, `sinon`, `assert`. - - **Concrete example — stashed enum/constant that is never consumed:** - ```javascript - // Before (app_before): global stash that nothing ever reads - this.BarColor = sap.ui.core.BarColor; // ← this.BarColor never used elsewhere - - // WRONG fix: importing the library just to keep the dead assignment - // sap.ui.define(["sap/ui/core/library"], function(coreLibrary) { - // this.BarColor = coreLibrary.BarColor; ← STILL dead code! - - // CORRECT fix: DELETE both the assignment and the import entirely - // (no sap/ui/core/library import, no this.BarColor line) - ``` - - **How to verify**: grep the entire project (JS files, XML views, JSON models) for any read of `this.BarColor`, `BarColor`, or `{BarColor}`. If the only occurrence is the assignment itself → it's dead code → delete it. +1. Convert dot-notation to slash-notation: `com.example.app.utils.Helper` → `"com/example/app/utils/Helper"` +2. Add to dependency array at the **beginning** (see Notes), add corresponding parameter +3. Remove the `var X = global.namespace.X;` line +4. If multiple global reads exist, add all as dependencies in one pass +5. Verify parameter name matches the module's short name +6. **Atomicity**: Every global→local replacement MUST be paired with a `sap.ui.define` dependency. Cycles introduced here are resolved later by `fix-cyclic-deps` +7. **Post-fix validation**: Grep for every introduced variable name — confirm it resolves to a parameter, var/let/const, or `sap.ui.require` call -**CRITICAL — Before replacing global access, read the target module's `return` statement:** +**Before replacing, read the target module's `return` statement:** -Not every module exports a usable value. Before replacing `global.namespace.Module` with a dependency parameter, open the target module file and check what it returns: - -| Module's `return` | Dependency parameter | How to use | -|---|---|---| -| Returns a class (`return MyClass;`) | `MyClass` | `new MyClass()` or `MyClass.staticMethod()` | -| Returns a wrapper (`return { getInstance: fn }`) | `MyModule` | `MyModule.getInstance()` — NOT `new MyModule()` | -| Returns an instance (`return new MyClass()`) | `myInstance` | `myInstance.method()` directly | -| Returns nothing (side-effect only) | Fix the module first — see below | Then import normally | - -**Side-effect modules (no AMD export) — fix the target module first:** - -Some libraries (especially custom reuse libraries) register on the global namespace but do NOT return a value. Instead of working around this with an unused `_` parameter, **fix the target module** to export properly: - -1. Open the target side-effect module -2. Identify the object being assigned to the global namespace (e.g., `com.example.lib.myLibrary = {...}`) -3. Add a `return` statement that returns that object -4. Remove the global namespace assignment - -```javascript -// BEFORE — side-effect module (no AMD export, registers on global): -sap.ui.define(["sap/base/Log"], function(Log) { - "use strict"; - com.example.lib.myLibrary = { - doSomething: function() { Log.info("done"); } - }; -}); - -// AFTER — proper AMD module with return: -sap.ui.define(["sap/base/Log"], function(Log) { - "use strict"; - var myLibrary = { - doSomething: function() { Log.info("done"); } - }; - return myLibrary; -}); -``` - -Then in the **consuming module**, import normally via the dependency parameter: - -```javascript -// Consuming module — use the dependency parameter, NOT the global -sap.ui.define([ - "com/example/lib/myLibrary", - "sap/base/Log" -], function(myLibrary, Log) { - "use strict"; - myLibrary.doSomething(); -}); -``` - -**How to detect side-effect modules:** The target module ends with `});` without a preceding `return`, or assigns to a global (`com.x.y = {...}`) without returning it. +| Module's `return` | How to use the dependency parameter | +|---|---| +| Returns a class | `new MyClass()` or `MyClass.staticMethod()` | +| Returns a wrapper | `MyModule.getInstance()` | +| Returns an instance | `myInstance.method()` directly | +| Returns nothing (side-effect only) | Fix the target module first — add a `return` | -**Procedure for all Case 1c fixes:** -1. Open the target module file -2. Find the `return` statement at the end of the `sap.ui.define` factory -3. Match the export shape to the table above -4. Write the replacement call accordingly +**Side-effect modules**: If the target ends with `});` without a `return`, open it, replace the global namespace assignment with a local var, and add `return varName;`. Then import normally in the consuming module. ### 2. sap.ui.getCore() Calls -**Problem**: `sap.ui.getCore()` returns the Core singleton, but `sap.ui.core.Core` is the class. - -**Note**: `attachInit` / `Core.ready()` is for the early boot phase of UI5, typically used in standalone scripts or index.html initialization, NOT inside controllers. When a controller's `onInit` is called, UI5 is already fully initialized. - -```javascript -// Before - in a standalone init script (NOT a controller) -sap.ui.getCore().attachInit(function() { - // Bootstrap application - new sap.m.Shell({ - app: new sap.ui.core.ComponentContainer({ ... }) - }).placeAt("content"); -}); -``` - -**Fix Strategy**: Use the modern replacement APIs via sap.ui.define. +**Problem**: `sap.ui.getCore()` is deprecated; its methods have moved to dedicated modules. ```javascript -// After - in a standalone init script -sap.ui.define([ - "sap/ui/core/Core", - "sap/m/Shell", - "sap/ui/core/ComponentContainer" -], function(Core, Shell, ComponentContainer) { - "use strict"; +// Before — standalone init script (NOT a controller) +sap.ui.getCore().attachInit(function() { ... }); - Core.ready().then(function() { - // Bootstrap application - new Shell({ - app: new ComponentContainer({ ... }) - }).placeAt("content"); - }); +// After +sap.ui.define(["sap/ui/core/Core"], function(Core) { + Core.ready().then(function() { ... }); }); ``` -**Common sap.ui.getCore() Method Replacements:** +**Note**: `Core.ready()` is for boot-phase init scripts. Inside controllers, UI5 is already initialized — don't use it there. -| Deprecated | Replacement Module | Replacement Call | -|------------|-------------------|------------------| +**Key replacements** (full table in `references/core-api-replacements.md`): + +| Deprecated | Module | Call | +|---|---|---| | `sap.ui.getCore().attachInit(fn)` | `sap/ui/core/Core` | `Core.ready().then(fn)` | -| `sap.ui.getCore().byId(id)` | `sap/ui/core/Element` | `Element.getElementById(id)` — BUT see Case 4a: inside a **controller**, if the ID contains the view prefix (e.g., `"container-myapp---viewName--controlId"`), prefer `this.byId("controlId")` instead | +| `sap.ui.getCore().byId(id)` | `sap/ui/core/Element` | `Element.getElementById(id)` — in controllers prefer `this.byId()` | | `sap.ui.getCore().getEventBus()` | `sap/ui/core/EventBus` | `EventBus.getInstance()` | | `sap.ui.getCore().getLibraryResourceBundle(lib)` | `sap/ui/core/Lib` | `Lib.getResourceBundleFor(lib)` | -| `sap.ui.getCore().loadLibrary(lib, {async:true})` | `sap/ui/core/Lib` | `Lib.load({name: lib})` | - -**Important: Most Core APIs Have Been Moved** - -In modern UI5, most methods on `sap.ui.core.Core` have been moved to dedicated modules. Use the **UI5 MCP Server's `get_api_reference` tool** to check deprecation status and find replacements. -For complete replacement tables including extended Core methods, removed APIs, and jQuery.sap.* replacements, read `references/core-api-replacements.md`. +Use the **UI5 MCP Server's `get_api_reference` tool** for additional Core method replacements. ### 3. sap.ui.core.Core Direct Access -**Problem**: Accessing the Core class directly vs the singleton. - -```javascript -// Before - CANNOT be auto-fixed -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - var Core = sap.ui.core.Core; - // ... -}); -``` - -**Fix Strategy**: Add the module to sap.ui.define dependencies. +Add `sap/ui/core/Core` to the dependency array and remove the global access: ```javascript -// After -sap.ui.define([ - "sap/ui/core/mvc/Controller", - "sap/ui/core/Core" -], function(Controller, Core) { - // Use Core directly -}); +// Before: var Core = sap.ui.core.Core; +// After: add "sap/ui/core/Core" to deps, use Core parameter directly ``` -### 4. jQuery/$ Global Access (All Standard jQuery APIs) +### 4. jQuery/$ Global Access -**Problem**: Using `jQuery` or `$` as a global without importing the jQuery module. - -**IMPORTANT**: The fix is NOT to replace jQuery API calls. **All standard jQuery APIs are fine to use** — both instance methods (`.find()`, `.addClass()`, etc.) and static methods (`jQuery.each()`, `jQuery.extend()`, `jQuery.proxy()`, etc.). The only issue is that `jQuery`/`$` must be loaded through a proper module dependency. - -**How to tell the difference**: `jQuery.sap.*` (with `.sap.`) = deprecated UI5 utility, must be replaced (see 4b below). `jQuery.*` (without `.sap.`) or `jQuery(...)` = standard jQuery, keep as-is, just add import. +**IMPORTANT**: The fix is adding the import, NOT replacing jQuery API calls. `jQuery.sap.*` (with `.sap.`) = deprecated, must be replaced (Case 4b). `jQuery.*` (without `.sap.`) or `jQuery(...)` = standard jQuery, keep as-is. ```javascript -// Before - CANNOT be auto-fixed (jQuery/$ used as global) -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - return Controller.extend("my.app.App", { - onAfterRendering: function() { - jQuery("#myElement").addClass("highlight"); - $(".container").css("display", "block"); - jQuery.each(aItems, function(i, item) { /* ... */ }); - var oMerged = jQuery.extend(true, {}, oDefaults, oSettings); - var fnCallback = jQuery.proxy(this._handleResult, this); - } - }); -}); -``` - -**Fix Strategy**: Add `sap/ui/thirdparty/jquery` to dependencies. Replace bare `$` with `jQuery`. Keep all jQuery API calls as-is — do NOT replace `jQuery.each` with `forEach`, do NOT replace `jQuery.extend` with `Object.assign`, do NOT replace `jQuery.proxy` with `Function.prototype.bind`. +// Before +jQuery("#el").addClass("x"); +$(".container").css("display", "block"); +jQuery.each(items, function(i, item) { ... }); -```javascript -// After - jQuery loaded as a proper dependency -sap.ui.define([ - "sap/ui/core/mvc/Controller", - "sap/ui/thirdparty/jquery" -], function(Controller, jQuery) { - return Controller.extend("my.app.App", { - onAfterRendering: function() { - jQuery("#myElement").addClass("highlight"); - jQuery(".container").css("display", "block"); - jQuery.each(aItems, function(i, item) { /* ... */ }); - var oMerged = jQuery.extend(true, {}, oDefaults, oSettings); - var fnCallback = jQuery.proxy(this._handleResult, this); - } - }); +// After — add dependency, rename $ to jQuery, keep all API calls unchanged +sap.ui.define([..., "sap/ui/thirdparty/jquery"], function(..., jQuery) { + jQuery("#el").addClass("x"); + jQuery(".container").css("display", "block"); + jQuery.each(items, function(i, item) { ... }); }); ``` -**Key rules:** -- Add `"sap/ui/thirdparty/jquery"` to the dependency array, name the parameter `jQuery` -- Replace `$` references with `jQuery` (since `$` is a global alias that won't exist once globals are removed) -- Do NOT replace any standard jQuery API calls — just add the import - -**NEVER replace standard jQuery methods with native equivalents. This is WRONG:** - -| WRONG (do NOT do this) | CORRECT (keep as-is) | -|---|---| -| `jQuery.proxy(fn, ctx)` → `fn.bind(ctx)` | Keep `jQuery.proxy(fn, ctx)` unchanged | -| `jQuery.each(arr, fn)` → `arr.forEach(fn)` | Keep `jQuery.each(arr, fn)` unchanged | -| `jQuery.extend({}, a, b)` → `Object.assign({}, a, b)` | Keep `jQuery.extend({}, a, b)` unchanged | -| `jQuery.isArray(x)` → `Array.isArray(x)` | Keep `jQuery.isArray(x)` unchanged | -| `jQuery.isEmptyObject(x)` → `Object.keys(x).length === 0` | Keep `jQuery.isEmptyObject(x)` unchanged | -| `jQuery.inArray(v, arr)` → `arr.indexOf(v)` | Keep `jQuery.inArray(v, arr)` unchanged | -| `jQuery.grep(arr, fn)` → `arr.filter(fn)` | Keep `jQuery.grep(arr, fn)` unchanged | -| `jQuery.map(arr, fn)` → `arr.map(fn)` | Keep `jQuery.map(arr, fn)` unchanged | -| `jQuery.type(x)` → `typeof x` | Keep `jQuery.type(x)` unchanged | -| `jQuery.trim(s)` → `s.trim()` | Keep `jQuery.trim(s)` unchanged | - -Standard jQuery APIs are **not deprecated in UI5**. UI5 ships jQuery as `sap/ui/thirdparty/jquery` and all standard jQuery methods remain fully supported. The only fix needed is adding the module dependency — never rewrite the API calls themselves. Replacing jQuery methods with native equivalents would change behavior (e.g., `jQuery.extend` does deep copy with `true` flag, `jQuery.each` handles both arrays and objects, `jQuery.proxy` preserves identity for later unbinding) and is not required by the linter. +**NEVER replace these standard jQuery methods** — they are not deprecated in UI5: `jQuery.each`, `jQuery.extend`, `jQuery.proxy`, `jQuery.isEmptyObject`, `jQuery.isArray`, `jQuery.inArray`, `jQuery.grep`, `jQuery.map`, `jQuery.type`, `jQuery.trim`. ### 4a. jQuery DOM Lookup for UI5 Controls → this.byId() -**Problem**: Using `jQuery("#--")` followed by `.control(0)` or `Element.closestTo()` to get a reference to a UI5 control inside a controller. This is fragile (depends on generated ID prefixes) and unnecessary when you're inside the controller that owns the view. - -```javascript -// Before — fragile jQuery lookup with hardcoded ID prefix -onAfterRendering() { - const avatarDOM = jQuery("#container-todo---app--avatar-profile"); - const avatarCtr = avatarDOM.control(0); // or Element.closestTo(avatarDOM[0]) - avatarCtr.setSrc(Helper.resolvePath('./img/logo_ui5.png')); -} -``` - -**Fix Strategy**: Replace with `this.byId("")`. The local ID is the part after the last `--` separator. This eliminates the need for both the jQuery import and the `Element.closestTo` import (if they are not used elsewhere in the file). - -```javascript -// After — clean controller API -onAfterRendering() { - const oAvatar = this.byId("avatar-profile"); - oAvatar.setSrc(Helper.resolvePath('./img/logo_ui5.png')); -} -``` - -**Detection patterns:** -- `jQuery("#--").control(0)` → `this.byId("")` -- `Element.closestTo(jQuery("#--")[0])` → `this.byId("")` -- `sap.ui.getCore().byId("")` inside a controller where the ID contains the view prefix → `this.byId("")` -- `Element.getElementById("")` inside a controller where the ID contains `--` (view prefix separator) → `this.byId("")` (strip everything up to and including the last `--`) +**Problem**: `jQuery("#prefix--id").control(0)` or `Element.closestTo()` to get a UI5 control inside a controller. -**IMPORTANT**: Apply Case 4a AFTER Case 2 (sap.ui.getCore().byId → Element.getElementById). If Case 2 produces `Element.getElementById("container-app---view--controlId")` inside a controller, immediately convert it to `this.byId("controlId")` in the same pass — do NOT leave the intermediate form. +**Detection patterns** — all collapse to `this.byId("")`: +- `jQuery("#--").control(0)` +- `Element.closestTo(jQuery("#--")[0])` +- `sap.ui.getCore().byId("")` where ID contains view prefix +- `Element.getElementById("")` where ID contains `--` -**After replacing**: If `jQuery` and/or `Element` are no longer used anywhere else in the file, remove them from the dependency array and function parameters. +The local ID is the part after the last `--`. After replacing, remove unused `jQuery`/`Element` imports. ### 4b. jQuery.sap.* Utility Access -**Problem**: Using `jQuery.sap.*` utility methods — these are deprecated APIs with dedicated replacement modules. This is a **different case** from plain jQuery DOM access above. +**Problem**: `jQuery.sap.*` calls are deprecated UI5 utilities with dedicated replacement modules. ```javascript -// Before - CANNOT be auto-fixed -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - return Controller.extend("my.app.App", { - onInit: function() { - jQuery.sap.log.info("Controller initialized"); - var sId = jQuery.sap.uid(); - } - }); -}); -``` - -**Fix Strategy**: Replace `jQuery.sap.*` calls with their dedicated replacement modules. +// Before +jQuery.sap.log.info("msg"); +var sId = jQuery.sap.uid(); -```javascript // After -sap.ui.define([ - "sap/ui/core/mvc/Controller", - "sap/base/Log", - "sap/base/util/uid" -], function(Controller, Log, uid) { - return Controller.extend("my.app.App", { - onInit: function() { - Log.info("Controller initialized"); - var sId = uid(); - } - }); +sap.ui.define([..., "sap/base/Log", "sap/base/util/uid"], function(..., Log, uid) { + Log.info("msg"); + var sId = uid(); }); ``` -**Common jQuery.sap.* Replacements:** - -Run `npx @ui5/linter --details` to get the suggested replacement module for each jQuery.sap.* API. For the complete table, read `references/core-api-replacements.md`. - -| Deprecated | Replacement Module | Replacement Call | -|------------|-------------------|------------------| -| `jQuery.sap.log.*` | `sap/base/Log` | `Log.info()`, `Log.error()`, etc. | -| `jQuery.sap.uid` | `sap/base/util/uid` | `uid()` | -| `jQuery.sap.extend(true, ...)` | See below | See below | -| `jQuery.sap.encodeHTML` | `sap/base/security/encodeXML` | `encodeXML(text)` | +Run `npx @ui5/linter --details` for suggested replacements. Full table in `references/core-api-replacements.md`. -**`jQuery.sap.extend` replacement decision:** -- If the first argument is `true` (deep copy): use `sap/base/util/merge` → `merge({}, obj1, obj2)`. **Note**: The module name is `merge`, NOT `deepExtend` — there is no `sap/base/util/deepExtend` module. -- If the merged objects are **flat** (single-level properties, no nested objects/arrays that need recursive copying), use native `Object.assign({}, obj1, obj2)` — no import needed. -- If the merged objects contain **nested objects** that must be deep-copied, use `sap/base/util/merge` → `merge({}, obj1, obj2)`. -- **Prefer `Object.assign`** unless deep copy is demonstrably required. Inspect the objects being merged — if all properties are primitives or you only need a shallow copy, `Object.assign` is the correct modern replacement. -- **NEVER convert `jQuery.sap.extend(...)` to `jQuery.extend(...)`** — that would introduce a new dependency on `sap/ui/thirdparty/jquery` for no reason. `jQuery.sap.extend` is a deprecated wrapper and must be replaced with either `Object.assign` or `merge()`, never with the raw jQuery equivalent. -- **NEVER use `sap/base/util/deepExtend`** — this module does NOT exist. The correct deep-copy module is `sap/base/util/merge`. +**`jQuery.sap.extend` decision:** +- Deep copy (`true` as first arg) → `sap/base/util/merge` → `merge({}, obj1, obj2)` +- Flat objects (single-level properties) → `Object.assign({}, obj1, obj2)` (no import needed) +- **NEVER** convert to `jQuery.extend(...)` (introduces unnecessary dependency) +- **NEVER** use `sap/base/util/deepExtend` (does NOT exist) ### 5. Conditional/Probing Global Access -**Problem**: Code checks if a global exists before using it. +**Problem**: Code checks if a global exists: `if (sap.ui.fl && sap.ui.fl.Utils) { ... }` -```javascript -// Before - CANNOT be auto-fixed (conditional access) -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - return Controller.extend("my.app.App", { - onInit: function() { - if (sap.ui.fl && sap.ui.fl.Utils) { - sap.ui.fl.Utils.getComponentClassName(this); - } - } - }); -}); -``` - -**Fix Strategy**: For modules that are always available in your target environment, add them as a `sap.ui.define` dependency (see Case 11 for runtime globals like `sap.ushell.Container`). For truly optional modules that may not be loaded, use synchronous `sap.ui.require` which returns `undefined` if the module is not loaded: +**Fix**: For always-available modules, add as `sap.ui.define` dependency. For truly optional modules, use synchronous `sap.ui.require`: ```javascript -// After — synchronous require for truly optional modules -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - return Controller.extend("my.app.App", { - onInit: function() { - var FlUtils = sap.ui.require("sap/ui/fl/Utils"); - if (FlUtils) { - FlUtils.getComponentClassName(this); - } - } - }); -}); +var FlUtils = sap.ui.require("sap/ui/fl/Utils"); +if (FlUtils) { FlUtils.getComponentClassName(this); } ``` -**Alternative**: For lazy loading, use async `sap.ui.require(["sap/ushell/Container"], function(Container) { ... }, function() { /* error */ })` with a callback. +For lazy loading, use async: `sap.ui.require(["module/path"], function(Mod) { ... })`. ### 6. Custom Namespace Definitions -**Problem**: Application defines its own namespace structure. - -```javascript -// Before - CANNOT be auto-fixed -window.mycompany = window.mycompany || {}; -window.mycompany.myapp = window.mycompany.myapp || {}; -window.mycompany.myapp.utils = { - helper: function() { ... } -}; -``` - -**Fix Strategy**: Convert to proper sap.ui.define module. - -```javascript -// After - mycompany/myapp/utils.js -sap.ui.define([], function() { - "use strict"; - - return { - helper: function() { ... } - }; -}); -``` - -Other files then consume it via `sap.ui.define(["mycompany/myapp/utils"], function(utils) { ... })`. +Same pattern as Case 1 but for non-SAP namespaces (`window.mycompany.myapp = {...}`). Convert to `sap.ui.define` module returning the object. Consumers import via dependency. ### 7. Binding Type Strings Without Import -**Problem**: Using type as string in binding without importing the module. - ```javascript -// Before - triggers no-globals (inside a controller) -var oInput = new Input({ - value: { - path: "/amount", - type: "sap.ui.model.type.Float" // Global reference as string - } -}); -``` - -**Fix Strategy**: Import the type module and use the class reference. +// Before — global reference as string +value: { path: "/amount", type: "sap.ui.model.type.Float" } -```javascript -// After - add "sap/ui/model/type/Float" to sap.ui.define dependencies -var oInput = new Input({ - value: { - path: "/amount", - type: new FloatType() // FloatType from dependency - } -}); +// After — import type module, use class reference +value: { path: "/amount", type: new FloatType() } // FloatType from "sap/ui/model/type/Float" ``` ### 8. Delete Expressions -**Problem**: Deleting properties from global namespace. - -```javascript -// Before - CANNOT be auto-fixed -delete sap.ui.core.someTempProperty; -``` - -**Fix Strategy**: This is usually a code smell. Either: -- Remove the code entirely if it's cleanup of old patterns -- If legitimately needed, use a local object instead of globals +`delete sap.ui.core.someTempProperty` — usually a code smell. Remove entirely or use a local object. ### 9. sap.ui.controller() — Controller Definition via Global Factory -**Problem**: Using the deprecated `sap.ui.controller()` global factory to define a controller. This is the two-argument form that **defines** a controller class. - -**Scope**: This case handles plain controller definitions in custom UI5 apps. It does NOT handle Fiori Elements V2 extensions using `registerControllerExtensions` — use `fix-fiori-elements-extensions` for those (the indicator is `sap.ui.controllerExtensions` in manifest.json or `registerControllerExtensions` in Component.js). +**Scope**: Plain controller definitions. NOT Fiori Elements V2 extensions (use `fix-fiori-elements-extensions` for those). -**Detection**: -```bash -grep -rn 'sap\.ui\.controller(' webapp/ --include="*.js" | grep -v "^\s*//" -``` -Categorize each match: -- **Two arguments** `sap.ui.controller("name", { ... })` — **definition**, fix it here -- **One argument** `sap.ui.controller("name")` — **instance lookup**, document in `MODERNIZATION-ISSUES.md` (sync-to-async refactoring required) - -#### Pattern A: Definition inside existing `sap.ui.define` wrapper +**Detection**: `grep -rn 'sap\.ui\.controller(' webapp/ --include="*.js"` +- Two arguments `sap.ui.controller("name", {...})` = **definition** → fix here +- One argument `sap.ui.controller("name")` = **instance lookup** → document in `MODERNIZATION-ISSUES.md` -The most common case: file already uses `sap.ui.define` but defines the controller via the deprecated global. +#### Pattern A: Inside existing sap.ui.define ```javascript // Before -sap.ui.define([ - "sap/m/MessageBox", - "sap/base/Log" -], function(MessageBox, Log) { - "use strict"; - - return sap.ui.controller("my.app.controller.Main", { - onInit: function() { - Log.info("initialized"); - }, - onPress: function() { - MessageBox.show("Hello"); - } - }); +sap.ui.define(["sap/m/MessageBox"], function(MessageBox) { + return sap.ui.controller("my.app.controller.Main", { ... }); }); -``` -```javascript -// After +// After — add Controller dep, replace factory with extend sap.ui.define([ "sap/ui/core/mvc/Controller", - "sap/m/MessageBox", - "sap/base/Log" -], function(Controller, MessageBox, Log) { - "use strict"; - - return Controller.extend("my.app.controller.Main", { - onInit: function() { - Log.info("initialized"); - }, - onPress: function() { - MessageBox.show("Hello"); - } - }); + "sap/m/MessageBox" +], function(Controller, MessageBox) { + return Controller.extend("my.app.controller.Main", { ... }); }); ``` -Steps: -1. Add `"sap/ui/core/mvc/Controller"` to the dependency array (first position) -2. Add `Controller` as the corresponding function parameter -3. Replace `sap.ui.controller("name", {` with `Controller.extend("name", {` - -#### Pattern B: Definition without `sap.ui.define` (legacy module system) - -Files using `jQuery.sap.declare`/`jQuery.sap.require` or no module wrapper at all. +#### Pattern B: Without sap.ui.define (legacy module system) ```javascript // Before jQuery.sap.declare("my.app.controller.Detail"); jQuery.sap.require("sap.ui.core.mvc.Controller"); +sap.ui.controller("my.app.controller.Detail", { onInit: function() { ... } }); -sap.ui.controller("my.app.controller.Detail", { - onInit: function() { - // ... - } -}); -``` - -```javascript // After -sap.ui.define([ - "sap/ui/core/mvc/Controller" -], function(Controller) { - "use strict"; - - return Controller.extend("my.app.controller.Detail", { - onInit: function() { - // ... - } - }); -}); -``` - -Steps: -1. Remove `jQuery.sap.declare` and `jQuery.sap.require` calls -2. Wrap in `sap.ui.define([...], function(...) { ... });` -3. Add all required dependencies (convert dot-notation to slash-notation paths) -4. Replace `sap.ui.controller("name", {` with `Controller.extend("name", {` -5. Add `return` before `Controller.extend(...)` so the module exports the controller class -6. Add `"use strict";` inside the factory function -7. **CRITICAL — Apply all inline fixes to the file body** (see "Apply ALL Applicable Cases in a Single Pass" section above): replace `jQuery("#prefix--id").control(0)` → `this.byId("id")`, replace `sap.ui.getCore().byId("prefix--id")` → `this.byId("id")`, replace `jQuery.sap.*` utilities → dedicated modules, replace inline `sap.ui.model.Filter` etc. → dependency imports - -#### Pattern C: Mixed file with both definition AND instance lookups - -A controller file defines itself with `sap.ui.controller("name", {...})` AND uses `sap.ui.controller("otherName")` (single argument) to look up other controller instances. - -**Action:** -- Fix the definition (Pattern A/B above) -- Leave the instance lookups untouched — they require manual sync-to-async refactoring (EventBus, shared services, or stored references) -- Document the instance lookups in `MODERNIZATION-ISSUES.md` - -#### Edge Cases - -**Missing `return` statement**: The old `sap.ui.controller()` API registered the controller globally as a side effect. `Controller.extend()` only returns the class — it doesn't register globally. If the original code doesn't return the result, you must add `return`: - -```javascript -// Before (broken after modernization without return) -sap.ui.define(["sap/ui/core/mvc/Controller"], function(Controller) { - "use strict"; - sap.ui.controller("my.app.controller.Main", { /* ... */ }); - // No return! Works with sap.ui.controller but NOT with Controller.extend -}); - -// After (fixed) sap.ui.define(["sap/ui/core/mvc/Controller"], function(Controller) { "use strict"; - return Controller.extend("my.app.controller.Main", { /* ... */ }); + return Controller.extend("my.app.controller.Detail", { onInit: function() { ... } }); }); ``` -**Module-level variables before definition**: Keep them as-is — just wrap the `Controller.extend(...)` with `return`. - -**Controller name must match file path**: The string in `Controller.extend("my.app.controller.Main", {...})` should match the file's module path. A file at `webapp/controller/Main.controller.js` in an app with namespace `my.app` should be `"my.app.controller.Main"`. If the existing name doesn't match the file path, keep the existing name (may be intentional). +Steps: Remove `jQuery.sap.declare`/`require`. Wrap in `sap.ui.define`. Convert dot-notation deps to slash-notation. Replace `sap.ui.controller` with `Controller.extend`. Add `return`. Add `"use strict"`. **Apply all inline fixes to file body** (see "Apply ALL" section). -**Key rules:** -- If the file already has `sap.ui.define`, do NOT wrap again — just add the `sap/ui/core/mvc/Controller` dependency if missing -- `sap.ui.controller("name", { ... })` (with object literal = **definition**) → `Controller.extend("name", { ... })` -- `sap.ui.controller("name")` (no object literal = **instantiation**) → document in `MODERNIZATION-ISSUES.md`, do NOT auto-fix -- If extending a custom base controller, import that base controller instead of `sap/ui/core/mvc/Controller` +#### Edge cases +- **Missing `return`**: `Controller.extend()` only returns the class — always add `return` before it +- **Module-level variables before definition**: Keep as-is, just wrap the extend call with `return` +- **Controller name must match file path**: Keep existing name even if mismatched (may be intentional) +- **Mixed file (definition + instance lookups)**: Fix the definition, document instance lookups in `MODERNIZATION-ISSUES.md` ### 10. jQuery.sap.declare/require — Legacy Module Definitions -**Problem**: Using deprecated `jQuery.sap.declare()` and `jQuery.sap.require()` for module management. +Same structural conversion as Case 9 Pattern B but for non-controller modules: ```javascript -// Before - CANNOT be auto-fixed (no-deprecated-api: jQuery.sap.declare, jQuery.sap.require) +// Before jQuery.sap.declare("my.app.util.Formatter"); jQuery.sap.require("sap.ui.core.format.DateFormat"); +my.app.util.Formatter = { formatDate: function(oDate) { ... } }; -my.app.util.Formatter = { - formatDate: function(oDate) { - var oDateFormat = sap.ui.core.format.DateFormat.getDateTimeInstance(); - return oDateFormat.format(oDate); - } -}; -``` - -**Fix Strategy**: Wrap in `sap.ui.define`, convert `jQuery.sap.require` calls to dependency array, remove global assignment, return the module object. - -```javascript // After -sap.ui.define([ - "sap/ui/core/format/DateFormat" -], function(DateFormat) { +sap.ui.define(["sap/ui/core/format/DateFormat"], function(DateFormat) { "use strict"; - - return { - formatDate: function(oDate) { - var oDateFormat = DateFormat.getDateTimeInstance(); - return oDateFormat.format(oDate); - } - }; + return { formatDate: function(oDate) { ... } }; }); ``` -**Key rules:** -- Remove all `jQuery.sap.declare()` statements — module names are inferred from file paths in AMD -- Convert `jQuery.sap.require("sap.m.Button")` to dependency `"sap/m/Button"` (dot notation → path notation) -- Remove global namespace assignment (`my.app.util.Formatter = {...}`) and `return` the object from the factory function -- If the file already has `sap.ui.define`, do NOT wrap again — merge remaining `jQuery.sap.require` calls into the existing dependency array -- If `jQuery.sap.require` is inside a function (dynamic/conditional), replace with `sap.ui.require(["sap/m/MessageBox"], function(MessageBox) { ... })` instead -- If the file has multiple `jQuery.sap.declare` calls or no clear single export, flag for manual review — do not attempt automatic modernization -- **After structural conversion, apply all inline fixes** (see "Apply ALL Applicable Cases in a Single Pass" section): replace `jQuery("#prefix--id").control(0)` → `this.byId("id")`, replace `sap.ui.getCore().byId(...)` → `this.byId(...)`, replace remaining `jQuery.sap.*` → dedicated modules, replace inline class references (e.g., `new sap.ui.model.json.JSONModel(...)`) → imported dependency variables +**Key rules**: Remove `jQuery.sap.declare`. Convert `jQuery.sap.require` to deps. Remove global assignment, `return` the object. If already has `sap.ui.define`, merge remaining requires into existing dep array. Dynamic/conditional requires → `sap.ui.require(["..."], callback)`. Multiple declares or unclear exports → flag for manual review. **Apply all inline fixes** (see "Apply ALL" section). ### 11. Runtime Globals as Module Imports -**Problem**: Runtime-provided modules like `sap.ushell.Container` are accessed via global namespace chains. Under strict AMD loading or Test Starter, these globals may not exist because the FLP shell isn't bootstrapped. +**Problem**: Runtime modules like `sap.ushell.Container` accessed via global namespace chains. ```javascript -// Before - global namespace chain access -sap.ui.define(["sap/ui/core/mvc/Controller"], function(Controller) { - return Controller.extend("com.example.app.controller.Main", { - onNavBack: function() { - if (sap.ushell && sap.ushell.Container) { - var oNav = sap.ushell.Container.getService("CrossApplicationNavigation"); - oNav.toExternal({ target: { shellHash: "#" } }); - } - }, - onSave: function() { - sap.ushell.Container.setDirtyFlag(true); - } - }); -}); -``` - -**Fix Strategy**: Add the runtime module as a `sap.ui.define` dependency. Replace all global namespace chain access with the imported variable. Convert guarded access patterns to guard on the imported variable. +// Before +if (sap.ushell && sap.ushell.Container) { + sap.ushell.Container.getService("CrossApplicationNavigation"); +} -```javascript -// After - proper module import -sap.ui.define([ - "sap/ushell/Container", - "sap/ui/core/mvc/Controller" -], function(Container, Controller) { - return Controller.extend("com.example.app.controller.Main", { - onNavBack: function() { - if (Container && Container.getService) { - var oNav = Container.getService("CrossApplicationNavigation"); - oNav.toExternal({ target: { shellHash: "#" } }); - } - }, - onSave: function() { - Container.setDirtyFlag(true); - } - }); +// After — add as dependency +sap.ui.define(["sap/ushell/Container", ...], function(Container, ...) { + if (Container && Container.getService) { + Container.getService("CrossApplicationNavigation"); + } }); ``` -**Test-side pattern**: Because UI5's `sap.ui.define` loader caches module return values, the test and app source both receive the same object reference. Stub the imported module directly — do NOT set up global namespace chains. `sinon` is globally available via the Test Starter infrastructure — no need to import it: +**Test-side**: Stub the imported module directly with `sinon`. Do NOT set up global namespace chains (`window.sap.ushell = {...}`). `sinon` and `QUnit` are Test Starter globals — no import needed. ```javascript -// WRONG — setting up global namespace in test -window.sap.ushell = { Container: { setDirtyFlag: function() {} } }; - -// CORRECT — stub the imported module (sinon is a Test Starter global) sap.ui.define(["sap/ushell/Container"], function(Container) { var oSandbox = sinon.createSandbox(); - // ... - QUnit.module("Navigation", { - afterEach: function() { oSandbox.restore(); } - }); - QUnit.test("setDirtyFlag is called", function(assert) { + QUnit.test("...", function(assert) { oSandbox.stub(Container, "setDirtyFlag"); - // ... trigger action ... - assert.ok(Container.setDirtyFlag.calledOnce); + // ... }); }); ``` -**Common runtime globals to convert:** `sap.ushell.Container` and other runtime-provided modules accessed via global namespace chains. Cross-reference `fix-linter-blind-spots` for the broader detection pass across all files. - ### 12. Sync XHR Guards After jQuery.sap.sjax Modernization -**Problem**: When modernizing `jQuery.sap.sjax` to native synchronous `XMLHttpRequest`, the resulting code blindly uses `xhr.responseText` or `JSON.parse(xhr.responseText)` without checking whether the request succeeded. If the target file doesn't exist or the server returns an error (e.g., 404 HTML), `JSON.parse` throws on HTML content. +After modernizing `jQuery.sap.sjax` to native `XMLHttpRequest`, always guard `xhr.responseText` with a status check: ```javascript -// Before — jQuery.sap.sjax (deprecated) -var oResponse = jQuery.sap.sjax({ - url: sUrl, - dataType: "json" -}); -if (oResponse.data) { ... } - -// After modernization (WRONG — no guard) -var xhr = new XMLHttpRequest(); -xhr.open("GET", sUrl, false); -xhr.send(); -var oData = JSON.parse(xhr.responseText); // crashes if file missing or 404 - -// After modernization (CORRECT — with guard) var xhr = new XMLHttpRequest(); xhr.open("GET", sUrl, false); xhr.send(); if (xhr.readyState === 4 && xhr.status === 200) { var oData = JSON.parse(xhr.responseText); - // ... use oData ... } else { Log.error("Failed to load: " + sUrl); } ``` -**Fix Strategy**: Always wrap `xhr.responseText` usage with a `readyState === 4 && status === 200` check. Choose the fallback based on context: - -| Context | Guard Pattern | Fallback | -|---------|--------------|----------| -| Mock server response handler | `if (xhr.readyState === 4 && xhr.status === 200)` | `oXhr.respondJSON(200, {}, JSON.stringify({"d": {"results": []}}))` | -| JSON.parse of response | `if (xhr.readyState === 4 && xhr.status === 200)` | Return empty object `{}` — lets existing `if (oResponse.data)` guards work | -| Init-time config/manifest loading | `if (xhr.readyState !== 4 \|\| xhr.status !== 200) { Log.error(...); return; }` | Early return with error log | -| Shared helper function | Check inside the helper, return `{}` on failure | All callers already guard with `oResponse.data` | - -**Detection**: After modernizing `jQuery.sap.sjax`, scan for `xhr.responseText` or `JSON.parse(xhr.responseText)` that is NOT preceded by a `readyState`/`status` check within the same block. +| Context | Fallback on failure | +|---------|---| +| Mock server response handler | `oXhr.respondJSON(200, {}, JSON.stringify({"d": {"results": []}}))` | +| JSON.parse of response | Return `{}` (lets existing `if (oResponse.data)` guards work) | +| Init-time config loading | Early return with `Log.error(...)` | ## CRITICAL: Apply ALL Applicable Cases in a Single Pass -When a file triggers Case 9 or Case 10 (structural conversion from legacy module system to `sap.ui.define`), you MUST also fix ALL other global-access patterns in the **same file** during the **same pass**. Do NOT stop after the structural conversion — scan the entire file body for: - -- Case 2: `sap.ui.getCore().byId("prefix--id")` → `this.byId("id")` (in controllers) -- Case 4a: `jQuery("#prefix--id").control(0)` → `this.byId("id")` (in controllers) -- Case 4b: `jQuery.sap.*` utilities → dedicated replacement modules -- Case 4: Remaining `jQuery`/`$` global references → add `sap/ui/thirdparty/jquery` import -- Case 3: `sap.ui.core.Core` direct access → add module dependency -- Case 1: `sap.ui.demo.myApp.Module` global namespace → use imported dependency +When a file triggers Case 9 or 10, fix ALL global-access patterns in the same pass: -The structural conversion (Case 9/10) creates the `sap.ui.define` wrapper, but the code INSIDE the controller methods still contains inline global patterns. These inline patterns MUST be fixed in the same operation. Do not leave them for a "second pass" — there is no second pass. - -**Checklist after Case 9/10 conversion:** -1. Are there any `jQuery("#...")` or `jQuery(...)` calls? → Apply Case 4a or Case 4 -2. Are there any `sap.ui.getCore().byId(...)` calls? → Apply Case 2 (and then Case 4a if inside a controller) -3. Are there any `jQuery.sap.*` calls remaining? → Apply Case 4b -4. Are there any `sap.ui.model.*`, `sap.m.*`, `sap.ui.core.*` inline class references? → Convert to dependency imports -5. Are there any app-namespace global references (e.g., `sap.ui.demo.todo.util.Helper`)? → Convert to dependency imports -6. After all replacements, are any imports now unused? → Remove them from the dependency array and parameters -7. Are there any `this.X = importedModule.X` assignments where `this.X` is never read elsewhere in the file or referenced in XML views? → DELETE the assignment AND the import — it's dead code (a stashed global reference that nothing consumes) +1. `jQuery("#...")` or `jQuery(...)` calls? → Case 4a or 4 +2. `sap.ui.getCore().byId(...)` calls? → Case 2, then Case 4a if in controller +3. `jQuery.sap.*` calls? → Case 4b +4. `sap.ui.model.*`, `sap.m.*`, `sap.ui.core.*` inline class references? → dependency imports +5. App-namespace global references? → dependency imports +6. Unused imports after replacements? → remove from dep array and parameters +7. `this.X = importedModule.X` where `this.X` is never read elsewhere? → DELETE (dead code) ## Implementation Steps -1. **Run linter with --details** to get replacement suggestions: - ```bash - npx @ui5/linter --details - ``` -2. **Identify the error pattern** from linter output -3. **Determine the case type** (assignment, getCore, jQuery, conditional, etc.) -4. **Apply the appropriate transformation**: - - Add required modules to `sap.ui.define` dependency array - - Add corresponding parameter names to the function - - Replace global access with the parameter variable - - After replacing jQuery DOM lookups with `this.byId()`, remove `jQuery`/`Element` imports if no longer referenced - - Remove dead code: unused variables, stale comments describing old patterns -5. **Update any dependent code** that expects the global -6. **Verify no other files depend on the global** if it was an assignment - -## Example Fix Session - -For a comprehensive before/after example combining multiple case types (jQuery.sap.*, jQuery DOM, conditional globals), read `references/example-fix-session.md`. +1. Run `npx @ui5/linter --details` to get replacement suggestions +2. Identify error pattern and determine case type +3. Apply the appropriate transformation (add deps, replace globals, remove dead code) +4. After replacing jQuery DOM lookups with `this.byId()`, remove unused imports +5. Verify no other files depend on a removed global assignment ## Notes -- **Dependency insertion position — critical**: Always add new dependencies and their corresponding function parameters **at the beginning** of their respective lists, not at the end or in alphabetical order. Many legacy UI5 files have mismatches between the dependency array length and the function parameter count (e.g., trailing side-effect imports without parameters, or arrays that drifted out of sync over time). Inserting in the middle or at the end can shift the mapping between existing dependencies and parameters, silently passing the wrong module to existing code. Inserting at the beginning preserves all existing positional mappings. +- **Dependency insertion position — critical**: Always add new dependencies at the **beginning** of the array (and corresponding parameters at the beginning of the function). Many legacy files have dep/param count mismatches (trailing side-effect imports without parameters). Inserting at the end shifts existing mappings; inserting at the beginning preserves them. ```javascript - // Before — existing code may have a dep/param mismatch (3 deps, 2 params) - sap.ui.define([ - "sap/ui/core/mvc/Controller", - "sap/m/MessageToast", - "some/sideEffect/Module" - ], function(Controller, MessageToast) { - var oCore = sap.ui.getCore(); // global access to fix - }); - - // After — new dependency added at the BEGINNING - sap.ui.define([ - "sap/ui/core/Element", - "sap/ui/core/mvc/Controller", - "sap/m/MessageToast", - "some/sideEffect/Module" - ], function(Element, Controller, MessageToast) { - var oControl = Element.getElementById("myId"); - }); + // Before (3 deps, 2 params — mismatch is common in legacy code) + sap.ui.define(["sap/ui/core/mvc/Controller", "sap/m/MessageToast", "some/sideEffect/Module" + ], function(Controller, MessageToast) { ... }); + + // After — new dep added at BEGINNING + sap.ui.define(["sap/ui/core/Element", "sap/ui/core/mvc/Controller", "sap/m/MessageToast", "some/sideEffect/Module" + ], function(Element, Controller, MessageToast) { ... }); ``` -- The function parameter names should match the module's default export name (e.g., `Log` for `sap/base/Log`) -- Some globals like `QUnit` and `sinon` are intentionally allowed in test files -- `sap.ui.define`, `sap.ui.require`, and `sap.ui.loader.config` are allowed globals -- Use `sap.ui.require("module/path")` (synchronous, returns undefined if not loaded) for optional runtime dependencies +- Parameter names should match the module's default export name (e.g., `Log` for `sap/base/Log`) +- `QUnit`, `sinon` are intentionally allowed globals in test files +- `sap.ui.define`, `sap.ui.require`, `sap.ui.loader.config` are allowed globals +- Use `sap.ui.require("module/path")` (sync, returns undefined if not loaded) for optional deps - Use `sap.ui.require(["module/path"], callback)` (async) for lazy loading +## Example Fix Session + +For a comprehensive before/after example combining multiple case types, read `references/example-fix-session.md`. + ## Related Skills -- **fix-fiori-elements-extensions**: For `sap.ui.controller()` in Fiori Elements V2 apps with `registerControllerExtensions` or manifest `sap.ui.controllerExtensions` — that's a different modernization path (ControllerExtension class + manifest registration + handler reference updates) -- **fix-pseudo-modules**: For `no-pseudo-modules` and `no-implicit-globals` errors (enum imports, DataType imports, OData expression functions), use fix-pseudo-modules -- **fix-control-renderer**: For renderer-specific issues (`no-deprecated-control-renderer-declaration`, `apiVersion`, `IconPool`, `rerender`), use fix-control-renderer -- **fix-xml-globals**: For `no-globals` in XML views/fragments (formatters, event handlers via `core:require`), use fix-xml-globals -- **fix-linter-blind-spots**: For runtime-breaking global namespace patterns the linter doesn't detect (app-specific namespaces outside `sap.*`), use fix-linter-blind-spots. Cases 1b and 1c overlap with patterns 1-4 in that skill. -- **fix-cyclic-deps**: When Case 1c fixes would create cyclic dependencies, use lazy `sap.ui.require` instead of normal `sap.ui.define` deps — see fix-cyclic-deps for cycle detection +- **fix-fiori-elements-extensions**: For `sap.ui.controller()` in Fiori Elements V2 apps with `registerControllerExtensions` or manifest `sap.ui.controllerExtensions` +- **fix-pseudo-modules**: For `no-pseudo-modules` and `no-implicit-globals` errors (enum imports, DataType imports, OData expression functions) +- **fix-control-renderer**: For renderer-specific issues (`no-deprecated-control-renderer-declaration`, `apiVersion`, `IconPool`, `rerender`) +- **fix-xml-globals**: For `no-globals` in XML views/fragments (formatters, event handlers via `core:require`) +- **fix-linter-blind-spots**: For runtime-breaking global namespace patterns the linter doesn't detect (app-specific namespaces outside `sap.*`). Cases 1b and 1c overlap with patterns 1-4 in that skill. +- **fix-cyclic-deps**: When Case 1c fixes would create cyclic dependencies, use lazy `sap.ui.require` instead of normal `sap.ui.define` deps diff --git a/plugins/ui5-modernization/skills/fix-xml-globals/SKILL.md b/plugins/ui5-modernization/skills/fix-xml-globals/SKILL.md index efbfcca3..1b520591 100644 --- a/plugins/ui5-modernization/skills/fix-xml-globals/SKILL.md +++ b/plugins/ui5-modernization/skills/fix-xml-globals/SKILL.md @@ -279,40 +279,14 @@ When multiple modules share the same class name, use descriptive aliases: ``` -### Formatters with Multiple Parameters +### Formatters with Multiple Parts / Expression Bindings -When a binding expression uses a formatter with multiple parts, the `core:require` is the same — only the global namespace in the formatter reference changes. +Multi-part bindings and expression bindings follow the same `core:require` pattern — just replace the dotted global with the local alias. Expression binding example: ```xml - - - - - -``` - -### Globals Accessed Inside Expression Bindings - -Expression bindings that reference globals via the `${...}` syntax also need `core:require`. - -```xml - - - - - + + + ``` ## App-Namespace Globals in XML diff --git a/plugins/ui5-modernization/skills/modernize-flp-sandbox/SKILL.md b/plugins/ui5-modernization/skills/modernize-flp-sandbox/SKILL.md index 23b1eca9..9bc291c9 100644 --- a/plugins/ui5-modernization/skills/modernize-flp-sandbox/SKILL.md +++ b/plugins/ui5-modernization/skills/modernize-flp-sandbox/SKILL.md @@ -23,30 +23,14 @@ All §6 rules cite this file rather than restating constraints inline. ## Scope — three layers -The skill operates in three layers. Each §6 subsection below is tagged with -one of them so it is always clear what is core and what is adjacent. - -1. **Core migration** — actions that are necessary for the app to boot - under New Sandbox. The skill performs them unconditionally and fails the - migration if they cannot be performed. Triggers: §6a, §6b, §6c, §6f. - -2. **Test infrastructure** — actions that get the existing test suite - running again under New Sandbox without changing test semantics. The - skill performs the safe ones (resource-root rebind, OPA bootstrap - extraction). For anything that touches test logic, it detects, reports, - and offers to apply on consumer request. Triggers: §6d, §6e, §6g, - §6h, §6j, §6l. - -3. **Adjacent migrations & advisories** — concerns that are surfaced *by* - the New Sandbox migration but are not *part of* it. The skill detects - and reports only; no rewrites are applied. - Triggers: +1. **Core migration** — actions necessary for the app to boot under New Sandbox. Performed unconditionally; fails migration if impossible. Triggers: §6a, §6b, §6c, §6f. + +2. **Test infrastructure** — gets the test suite running again without changing test semantics. Safe rewrites applied unconditionally (resource-root rebind, OPA bootstrap extraction). Anything touching test logic: detect, report, offer on request. Triggers: §6d, §6e, §6g, §6h, §6j, §6l. + +3. **Adjacent migrations & advisories** — concerns surfaced *by* the migration but not *part of* it. Detect and report only; no rewrites applied. - QUnit 1.x → 2.x patterns (§6i) — advisory. - Shell-feature tests (§6j, §6l) — advisory. - - Note: deprecated ushell service usage (§6k) is detected as part of - the core pass and reported as "Manual action required" — it is a hard - runtime block, not an advisory. + - Deprecated ushell services (§6k) — detected in core pass, reported as "Manual action required" (hard runtime block). ## 1. Detect App Root @@ -56,19 +40,18 @@ If the user provided a path argument, use it. Otherwise, detect from CWD: - If not found, tell the user: "Could not detect a UI5 app root. Please run from inside the app directory or provide a path: `/modernize-flp-sandbox ~/path/to/app`" -Set `APP_ROOT` to the resolved absolute path. All file operations use this as the base. +Set `APP_ROOT` to the resolved absolute path. ## 2. Ensure Clean State (Pre-Migration) Before touching any files, secure a rollback point. -**If git repository detected** (`git -C $APP_ROOT rev-parse --git-dir` succeeds): +**If git repository detected:** ```bash cd $APP_ROOT git stash push -m "modernize-flp-sandbox: pre-migration backup $(date +%Y%m%d-%H%M%S)" ``` -Save the stash ref for rollback. If `git stash` fails (nothing to stash), note it — the -working tree is already clean. +Save the stash ref for rollback. If nothing to stash, the tree is already clean. **If no git repository:** ```bash @@ -76,155 +59,80 @@ mkdir -p $APP_ROOT/webapp/test/.migration-backup cp $APP_ROOT/webapp/test/flpSandbox*.html $APP_ROOT/webapp/test/.migration-backup/ 2>/dev/null cp $APP_ROOT/webapp/test/fioriSandboxAppConfig.json $APP_ROOT/webapp/test/.migration-backup/ 2>/dev/null ``` -Save the backup path for rollback. ## 3. Analyze -Read all HTML files under `$APP_ROOT/webapp/test/` (recursive, including subdirectories) and build a pattern inventory: +Read all HTML files under `$APP_ROOT/webapp/test/` (recursive) and build a pattern inventory. -**Detect legacy sandbox HTML files** — a file is a legacy sandbox HTML if it contains ANY of: -- `id="sap-ushell-bootstrap"` (sandbox bootstrap script tag), OR -- `window["sap-ushell-config"]` assignment, OR -- `src=` referencing `sandbox2.js` (intermediate New Sandbox old-style bootstrap — predates `SandboxBootTask.js`) - -The `sandbox2.js` pattern indicates a partial/old-style New Sandbox migration that still needs updating to the current `SandboxBootTask.js`-based approach. Treat it identically to the other legacy patterns. +**Detect legacy sandbox HTML files** — a file is legacy if it contains ANY of: +- `id="sap-ushell-bootstrap"` (sandbox bootstrap script tag) +- `window["sap-ushell-config"]` assignment +- `src=` referencing `sandbox2.js` (intermediate New Sandbox old-style bootstrap — predates `SandboxBootTask.js`; treat identically to other legacy patterns) **OPA test-page (does NOT receive SandboxBootTask / boot-manifest):** -A file matches the legacy sandbox detection criteria above AND ALSO -contains ALL of: -- `QUnit.config` or `QUnit.config.autostart` assignment, OR `window.QUnit = {` pattern -- `sap.ui.require(` with entries from `"sap/ui/qunit/`, `"sap/ui/test/`, or `"sap/ui/thirdparty/qunit` - -These files are NOT added to the legacy HTML migration list. Instead, they receive a different treatment composed of two pieces: (a) §6g extracts the inline OPA bootstrap into a sibling JS file and rewrites the HTML attributes, (b) §6i detects QUnit 1.x patterns in the journey/page modules and hands off to the `modernize-flp-sandbox-qunit` sub-skill on consumer consent. The HTML edits performed in §6g specifically: -- Remove ` @@ -305,115 +167,59 @@ Create `webapp/test/Test.qunit.html`: ``` -This single file replaces ALL individual test HTML files. Test Starter uses URL query parameters to select which test to run. - ## Phase 3: Update testsuite.qunit.html -Replace the contents of `webapp/test/testsuite.qunit.html`: - ```html QUnit test suite for <NAMESPACE-WITH-DOTS> - - - + ``` -This replaces `qunit-redirect.js` or `sap-ui-core.js` bootstrapping with `createSuite.js`. - ## Phase 4: Modernize Unit Test JS Files -### 4.0 FIRST — Identify and delete redundant aggregators +### 4.0 FIRST — Delete redundant aggregators -**Before converting any file**, scan `webapp/test/unit/` for redundant aggregators. A redundant aggregator is a JS file that: -- Uses `sap.ui.require([...], function() { QUnit.start(); })` to load other test modules, OR -- Uses `sap.ui.define([...])` to list dependencies with no test body, OR -- Contains NO actual test logic (`QUnit.module`, `QUnit.test`, `assert.*` calls) +A redundant aggregator has NO actual test logic — only `sap.ui.require([deps], function() { QUnit.start(); })` or load-only `sap.ui.define`. `QUnit.config.autostart = false` + `QUnit.start()` are boot scaffolding, NOT test logic. -**IMPORTANT**: `QUnit.config.autostart = false` and `QUnit.start()` are NOT test logic — they are boot scaffolding. A file that ONLY does `sap.ui.require([deps], function() { QUnit.start(); })` is a redundant aggregator, even though it mentions QUnit. - -Common filenames: `allTests.js`, `AllTests.js`, `legacyTests.qunit.js`, `allTests.qunit.js` — but ANY file matching this "load-only, no tests" pattern is a redundant aggregator. - -**Action**: Note the test modules they load (these will go into `unitTests.qunit.js`), then **DELETE** the aggregator file immediately. Do NOT convert it to `sap.ui.define` format. Do NOT add QUnit.test stubs. Do NOT keep it as a test entry. Also delete its companion HTML file (e.g., `legacyTests.qunit.html`). DELETE BOTH FILES. - -Example — this is a redundant aggregator (DELETE both .js and .html): -```javascript -QUnit.config.autostart = false; -sap.ui.require([ - "my/app/test/unit/controller/App.controller" -], function() { - "use strict"; - QUnit.start(); -}); -``` -It has no `QUnit.module`/`QUnit.test` of its own — it just loads another module and starts QUnit. Delete it. +**Action**: Note the test modules they load (for `unitTests.qunit.js`), then **DELETE** both `.js` and companion `.html` immediately. ### 4.1 Convert and rename real test files -Unit test JS files that **contain actual test logic** (`QUnit.module`, `QUnit.test`, `assert.*`) and use `Core.ready()`, `Core.attachInit()`, or `sap.ui.require` with `QUnit.start()` need TWO changes: - -1. **Rename the file** to add `.qunit.js` suffix (e.g., `App.controller.js` → `App.controller.qunit.js`) -2. **Convert the content** to `sap.ui.define` format (remove QUnit.config.autostart, Core.ready wrappers) +Files with actual test logic (`QUnit.module`, `QUnit.test`, `assert.*`) need: +1. **Rename** to `.qunit.js` suffix (e.g., `App.controller.js` → `App.controller.qunit.js`) +2. **Convert** to `sap.ui.define` (remove `QUnit.config.autostart`, `Core.ready` wrappers) -**⚠️ CRITICAL — File Rename**: Every real unit test file MUST be renamed to `.qunit.js` suffix. This is required because Test Starter resolves test entries by appending `.qunit` to the module path. Without the rename, the test cannot be found at runtime. - -Examples: -- `controller/App.controller.js` → `controller/App.controller.qunit.js` -- `model/formatter.js` → `model/formatter.qunit.js` -- `util/Helper.js` → `util/Helper.qunit.js` - -**Before** — old style with Core.ready (`webapp/test/unit/controller/App.controller.js`): ```javascript +// Before (App.controller.js) QUnit.config.autostart = false; sap.ui.getCore().attachInit(function() { - "use strict"; - sap.ui.require([ - "my/app/model/formatter" - ], function(formatter) { + sap.ui.require(["my/app/model/formatter"], function(formatter) { QUnit.module("formatter"); - QUnit.test("formatValue", function(assert) { - assert.equal(formatter.formatValue(1), "One"); - }); + QUnit.test("formatValue", function(assert) { ... }); }); }); -``` -**After** — Test Starter style (`webapp/test/unit/controller/App.controller.qunit.js`): -```javascript -sap.ui.define([ - "my/app/model/formatter" -], function(formatter) { +// After (App.controller.qunit.js) +sap.ui.define(["my/app/model/formatter"], function(formatter) { "use strict"; - QUnit.module("formatter"); - QUnit.test("formatValue", function(assert) { - assert.equal(formatter.formatValue(1), "One"); - }); + QUnit.test("formatValue", function(assert) { ... }); }); ``` ### 4.2 Create unitTests.qunit.js aggregator -The main testsuite entry `"unit/unitTests"` resolves to `unit/unitTests.qunit.js`. This file must directly list all **real** unit test modules (files with QUnit.module/QUnit.test). - -Build the list from: -- Test modules extracted from deleted aggregators (Step 4.0) -- Any additional `.qunit.js` files in `webapp/test/unit/` that contain actual tests +Directly lists all real unit test modules (from deleted aggregators + any additional `.qunit.js` files): -Do NOT include deleted aggregator files in this list. - -**After** (`unitTests.qunit.js` — directly lists all tests): ```javascript sap.ui.define([ "./controller/Main.qunit", @@ -421,54 +227,37 @@ sap.ui.define([ ]); ``` -Key rules for the aggregator: -- Use **relative paths** starting with `./`, not absolute namespace paths -- Add the **`.qunit` suffix** to each dependency (without `.js`) because the actual files were renamed to `.qunit.js` in Step 4.1 -- Example: if file was renamed to `controller/App.controller.qunit.js`, reference it as `"./controller/App.controller.qunit"` +Rules: relative `./` paths, include `.qunit` suffix (without `.js`). ### jsUnitTestSuite conversion -If the old `testsuite.qunit.js` used `jsUnitTestSuite`, it's already replaced in Phase 1. Delete the old content. +If old `testsuite.qunit.js` used `jsUnitTestSuite`, it's already replaced in Phase 1. Delete old content. ## Phase 5: Modernize OPA Tests -This phase differs based on the detected pattern. Read the full instructions in the corresponding reference file. - ### Pattern A — Single HTML + AllJourneys Read `references/pattern-a-modernization.md` for detailed instructions. -Summary: -1. **Create OpaSetup.js** from AllJourneys.js — extract `Opa5.extendConfig` and all page object/utility imports. OpaSetup.js must NOT import `sap/ui/test/opaQunit` — that module belongs in each individual journey file. -2. **Rename journey files** to `.qunit.js` suffix so Test Starter can resolve them without a `module` override -3. **Update journey files** — add OpaSetup as a side-effect dependency using relative path `"./OpaSetup"` (same directory). Do NOT use `test-resources/` for same-directory imports. -4. **Handle autoWait overrides** — journeys needing `autoWait: false` get a per-journey `Opa5.extendConfig` override -5. **Preserve testLibs config** — Fiori Elements `testLibs` settings move to OpaSetup.js +1. **Create OpaSetup.js** from AllJourneys.js — extract `Opa5.extendConfig` + page object imports. OpaSetup MUST NOT import `sap/ui/test/opaQunit`. +2. **Rename journey files** to `.qunit.js` +3. **Update journeys** — add `"./OpaSetup"` as side-effect dependency (relative path) +4. **Handle autoWait overrides** — per-journey `Opa5.extendConfig` +5. **Preserve testLibs config** in OpaSetup.js -**Correct OpaSetup.js structure** (page objects use `test-resources/`, but opaQunit is absent): ```javascript -sap.ui.define([ - "sap/ui/test/Opa5", - "test-resources//integration/pages/App" +// OpaSetup.js (no opaQunit!) +sap.ui.define(["sap/ui/test/Opa5", "test-resources//integration/pages/App" ], function(Opa5) { "use strict"; - - Opa5.extendConfig({ - viewNamespace: ".view.", - autoWait: true - }); + Opa5.extendConfig({ viewNamespace: ".view.", autoWait: true }); }); -``` -**Correct journey file structure** (opaQunit here, OpaSetup via relative path): -```javascript -sap.ui.define([ - "sap/ui/test/opaQunit", - "sap/ui/test/Opa5", - "./OpaSetup" +// Journey file (opaQunit here, OpaSetup relative) +sap.ui.define(["sap/ui/test/opaQunit", "sap/ui/test/Opa5", "./OpaSetup" ], function(opaTest, Opa5) { "use strict"; - // ... opaTest(...) calls + // opaTest(...) calls }); ``` @@ -476,153 +265,109 @@ sap.ui.define([ Read `references/pattern-b-modernization.md` for detailed instructions. -Summary: -1. **Inventory utility modules** — find all modules that call `Opa5.createPageObjects` (side-effect imports) -2. **Create OpaSetup.js** — consolidate all utility imports + `Opa5.extendConfig` from the HTML files -3. **Rename journey files** to `.qunit.js` suffix and add OpaSetup as a side-effect dependency -4. **Handle autoWait overrides** — use the parse script's `autoWaitFalseFiles` list -5. **Handle multi-module HTML files** — when a legacy `*.qunit.html` loads more than one journey module in a single `sap.ui.require`, emit ONE testsuite entry per module. Never invent a synthetic combined name (e.g. `Combined`) — the file does not exist and the resulting entry is dangling. The parse script does this automatically via `_fromMultiModuleHtml`. Halt if any of the loaded modules has no corresponding `.qunit.js` file under `webapp/test/`. See `references/pattern-b-modernization.md` Step 6. - - -## Phase 5b: Migrate in-window OPA launcher to bare-Component iframe - -**Run this phase only when Phase 0.2 reported `needsIframeMigration: true`** (i.e. `launcher === "in-window"` AND `flpSandbox === true`). Skip entirely for any other combination — including plain in-window apps with no FLP sandbox load, where `iStartMyUIComponent` should stay as-is. +1. **Inventory utility modules** calling `Opa5.createPageObjects` +2. **Create OpaSetup.js** — consolidate utility imports + `Opa5.extendConfig` +3. **Rename journeys** to `.qunit.js`, add OpaSetup dependency +4. **Handle autoWait overrides** from parse script's `autoWaitFalseFiles` +5. **Multi-module HTML files** — emit ONE testsuite entry per module (never invent `*Combined` names). See `references/pattern-b-modernization.md` Step 6. -Phase 5b assumes Phase 5 has already produced `OpaSetup.js`, renamed journey files, and the main `testsuite.qunit.js` — it then rewrites the launcher path so journeys run inside a fresh same-origin iframe loading the Component directly (no FLP shell). +## Phase 5b: Migrate in-window OPA to bare-Component iframe -Read `references/pattern-u-iframe-migration.md` for the full step-by-step instructions. Summary: +**Run only when Phase 0.2 reported `needsIframeMigration: true`.** -1. **Create `webapp/test/integration/opaIframe.qunit.html` + `opaIframeBoot.js`** — bare-Component iframe entry. HTML loads `sap/ushell` sandbox.js for API stubs but defines no `sap-ushell-config`, so no FLP shell renderer is built. Bootstrap uses `data-sap-ui-oninit="module:/test/integration/opaIframeBoot"` (no inline `` with ``. Even trivial config objects or debug flags must be extracted, not removed — removing them is a functional regression. - -Launch sub-agents in parallel for files with `csp-unsafe-inline-script` errors. Use the sub-agent prompt template, `{skill-name}=fix-csp-compliance`. Do NOT fix CSP issues inline from the main agent — the skill contains extraction patterns the main agent doesn't have in context. +Launch sub-agents for files with `csp-unsafe-inline-script`. Use template with `fix-csp-compliance`. ### Phase 5 commit + gate -- Run `npx @ui5/linter --details` for the final error count. -- Commit: `fix: enforce CSP compliance` -- Run the verification gate. +Final linter run for error count. -## Verification Gate (post-phase, every phase) +## Verification Gate -After every phase commit, run the gate matching the mode chosen in Phase 0. +After every phase commit, run the gate matching the mode chosen in Phase 0. **Use 600000ms timeout** for all test commands. ### Full autonomous -**Never skip tests.** The user chose full autonomous because they want verified correctness at every phase boundary. You must run the build and tests before proceeding to the next phase, every single time — no exceptions, no deferring to "after the next phase". If no build/test command is available, fall back to `npx @ui5/linter --details` as the minimum verification — but never proceed without running something. - -**Use a 600000ms (10 minute) timeout** for all build and test commands. These commands (especially Maven builds and headless QUnit runs) routinely exceed the default 2-minute timeout. +**Delegate to a sub-agent.** Print `⏳ Phase {N} gate: launching test sub-agent...` -**Delegate to a sub-agent.** Launch a sub-agent to run the build, run the tests, and debug failures. This keeps the orchestrator's context clean and lets the sub-agent focus on test output analysis. +Sub-agent instructions: +1. Read `references/build-and-test-commands.md` → detect Maven vs npm +2. Run the test command (Maven §1.3 or npm test). **Linter-only is NOT verification** unless no test command exists. +3. **Maven: exit code 0 ≠ all tests passed.** Check output for failure summaries AND `target/surefire-reports/`. +4. On pass → report `✅ TESTS OK` +5. On fail → analyze, attempt minimal fix (broken import/path/assertion only — never modernize code), re-run. Stop after 3 retries or 1 failed debug. +6. **Scope constraint:** Only fix failing tests caused by current phase. Never apply other-phase modernizations. -**Before launching the sub-agent**, print: `⏳ Phase {N} gate: launching test sub-agent...` - -The sub-agent prompt: - -``` -Run the verification gate for Phase {N} of a UI5 modernization workflow. - -Project root: {project-path} - -Build & test reference: {skills-dir}/modernize-ui5-app/references/build-and-test-commands.md - -CRITICAL REQUIREMENTS: -- You MUST execute the actual test command (Maven headless QUnit or npm test). A successful - build alone is NOT verification. Running only the linter is NOT verification. -- The gate requires BOTH a passing build AND passing tests. Do not report success unless - you have actually run the test command and confirmed all tests passed. -- IMPORTANT for Maven: exit code 0 does NOT guarantee all tests passed. Maven returns 0 - as long as the BUILD succeeds. You MUST check the end of the command output for failing - test summaries AND check `target/surefire-reports/` (if it exists) for detailed results. -- If you skip tests or only verify via build/linter, you have FAILED your task. -- EXCEPTION: If the build & test reference determines there is NO test command available - (npm project with no test script in package.json), linter-only verification IS acceptable. - In that case, run `npx @ui5/linter --details` and report: - "⚠️ NO TEST COMMAND AVAILABLE — verified via linter only (N errors)." - -SCOPE CONSTRAINT — DO NOT EXCEED: -- Your ONLY job is to run the tests, and if tests fail, fix the FAILING TESTS. -- NEVER fix linter errors, modernize code, or apply changes that belong to other phases. -- When debugging a test failure, ONLY make the minimal change needed to make the test pass - (e.g., fix an import path, adjust a test assertion, correct a module reference). -- Fixes for test breakage caused by the CURRENT phase's changes ARE in scope. For example: - if Phase 2 renamed a manifest property and a test reads that property, updating the test - to use the new name is a valid fix. -- Fixes that require applying a DIFFERENT phase's modernization pattern are NOT in scope. - Report those as "❌ FAILED" — do NOT apply the modernization fix yourself. -- You are a test runner, not a modernizer. Stay in your lane. - -Log progress at every step so the user has feedback during execution. - -Instructions: -1. Read the build & test reference above. Follow its "Project Type Detection" flow to determine - whether this is a Maven or npm project. -2. For Maven projects: §1.3 is self-contained (compiles + tests in one command). Skip §1.2 - (that's an interactive dev server). For npm projects: use the build and test scripts from - package.json (§2). -3. Print: "⏳ Phase {N} gate: running tests..." -4. Run the test command with a 600000ms timeout. - - Maven: §1.3 (`mvn clean verify -P execute.qunit ...`) - - npm: the test script from package.json (e.g., `npm test`) -5. Determine test result — IMPORTANT for Maven: exit code 0 does NOT guarantee all tests - passed. Maven returns 0 as long as the BUILD succeeds, even if individual QUnit tests - failed. You MUST check the end of the command output for failing test summaries AND - check `target/surefire-reports/` (if it exists) for detailed test results. Only report - tests as passed if both the output shows no failures AND no surefire report indicates - red tests. -6. Print test result: "✅ Tests passed (N tests)." or "❌ Tests failed: N of M red." -7. If tests pass → report: "✅ TESTS OK" -8. If tests fail: - a. Print: "🔍 Analyzing failure..." - b. Capture the failing test error output. - c. Print: "🔧 Attempting fix: {brief description of what you're fixing}" - d. Only fix the immediate test failure (broken import, wrong path, missing dependency). - Do NOT fix linter warnings, modernize APIs, or apply changes from other phases. - e. Print: "🔄 Re-running tests..." - f. Re-run the test command with a 600000ms timeout. - g. If it now passes → report: "✅ Fix applied, gate passes." Include what was fixed. - h. If it still fails → report: "❌ FAILED" with: - - Test failure summary (≤20 lines) - - What was attempted - - Suggested next action -9. If you have retried 3 times for the same failure, stop and report. - -For Maven projects: if tests fail with ClassNotFoundException or test-resources/ 404s, -consult the build & test reference §1.4 and references/SAPUI5_Local_Build.md §1.2. -``` - -**After the sub-agent returns:** -- If "✅" → print "✅ Phase {N} gate: tests OK — continuing to Phase {N+1}." → proceed. -- If "❌" → print the failure summary and ask: "Continue with next phase / retry / abort?" +After return: ✅ → proceed. ❌ → print summary, ask user. ### Half autonomous -**Delegate to a sub-agent.** Launch a sub-agent to run the build and tests, then report results back to the orchestrator for user review. - -**Before launching the sub-agent**, print: `⏳ Phase {N} gate: launching test sub-agent...` - -The sub-agent prompt: - +Same sub-agent but do NOT debug failures. Report results only: ``` -Run the verification gate for Phase {N} of a UI5 modernization workflow. - -Project root: {project-path} - -Build & test reference: {skills-dir}/modernize-ui5-app/references/build-and-test-commands.md - -CRITICAL REQUIREMENTS: -- You MUST execute the actual test command (Maven headless QUnit or npm test). A successful - build alone is NOT verification. Running only the linter is NOT verification. -- The gate requires BOTH a passing build AND passing tests. Do not report success unless - you have actually run the test command and confirmed all tests passed. -- IMPORTANT for Maven: exit code 0 does NOT guarantee all tests passed. Maven returns 0 - as long as the BUILD succeeds. You MUST check the end of the command output for failing - test summaries AND check `target/surefire-reports/` (if it exists) for detailed results. -- If you skip tests or only verify via build/linter, you have FAILED your task. -- EXCEPTION: If the build & test reference determines there is NO test command available - (npm project with no test script in package.json), linter-only verification IS acceptable. - In that case, run `npx @ui5/linter --details` and report: - "⚠️ NO TEST COMMAND AVAILABLE — verified via linter only (N errors)." - -Log progress at every step so the user has feedback during execution. - -Instructions: -1. Read the build & test reference above. Follow its "Project Type Detection" flow to determine - whether this is a Maven or npm project. -2. For Maven projects: §1.3 is self-contained (compiles + tests in one command). Skip §1.2 - (that's an interactive dev server). For npm projects: use the build and test scripts from - package.json (§2). -3. Print: "⏳ Phase {N} gate: running tests..." -4. Run the test command with a 600000ms timeout. - - Maven: §1.3 (`mvn clean verify -P execute.qunit ...`) - - npm: the test script from package.json (e.g., `npm test`) -5. Determine test result — for Maven: do NOT rely on exit code alone. Check the end of the - output for test failure summaries and check `target/surefire-reports/` if it exists. -6. Report results in this exact format: - ✅/❌ Tests: N passed, N failed, N skipped - Failed test names (≤10, then "...and X more") -6. Do NOT attempt to debug or fix failures — just report. +✅/❌ Tests: N passed, N failed, N skipped +Failed test names (≤10) ``` -**After the sub-agent returns:** -1. Print the sub-agent's structured report to the user. -2. Wait for the user. Acceptable user inputs: "continue", "skip phase", "run tests", "abort". -3. Do not attempt to debug or fix unless the user gives explicit instructions. +Wait for user: "continue" / "skip phase" / "run tests" / "abort". ### Manual -``` -1. Print a one-line phase summary: "Phase {N} done. Files modified: {count}. Errors fixed: {count}. Deferred: {count}." -2. Wait for the user to type "continue", verify externally, or give corrective input. -``` - -**Important: the agent does NOT re-ask which mode to use.** The mode was set in Phase 0 and applies to every gate. +Print: `Phase {N} done. Files: {count}. Fixed: {count}. Deferred: {count}.` +Wait for user "continue". ## Sub-Agent Prompt Template (Phases 1–5) -This template is used by every phase that delegates to skill sub-agents. Phase 3.2 (blind-spots) and 3.3 (cycles) have inline prompts above because they're single-shot global operations. - -**Placeholders:** -- `{project-path}` — absolute path to the UI5 project root -- `{skills-dir}` — absolute path to the parent of this skill's directory (resolved once by the orchestrator) -- `{skill-name}` — the skill folder name (e.g. `fix-js-globals`) -- `{file-path}` — target file(s) -- `{errors}` — linter errors for those files (rule, line, message) - ``` Fix UI5 linter errors in the following file(s) using the {skill-name} skill. @@ -593,33 +266,27 @@ Errors to fix: {errors} Instructions: -1. Read the skill at: {skills-dir}/{skill-name}/SKILL.md -2. Pay special attention to any "Key Rules" section at the top — these are load-bearing constraints from past failures. -3. Read any reference files mentioned in the skill. +1. Read {skills-dir}/{skill-name}/SKILL.md +2. Pay special attention to "Key Rules" — load-bearing constraints from past failures. +3. Read reference files mentioned in the skill. 4. Read the affected file(s). -5. Apply the fix patterns from the skill EXACTLY as documented — do not apply general web development best practices that conflict with the skill. -6. After fixing, verify each error is resolved. -7. Report back CONCISELY: - - Files modified (paths only) - - Count of errors fixed - - Errors that could NOT be fixed (one line each): - `{file}:{line} | {rule} | {reason} | {suggested fix}` +5. Apply fix patterns EXACTLY as documented. +6. Verify each error is resolved. +7. Report: files modified, errors fixed, unfixable errors ({file}:{line} | {rule} | {reason}). ``` ### Sub-agent execution rules -- **Foreground mode only.** Do NOT use `run_in_background: true`. Launch all sub-agents for a phase step in a SINGLE message with multiple Agent tool calls — this blocks the main agent until all return, preventing mid-edit interference. -- **Cap at ~8 concurrent sub-agents per message.** If more files, batch sequentially — do NOT stop after one batch. -- **Group related files into one sub-agent.** A controller and its XML view both needing `core:require` changes should share a sub-agent. -- **No validation between sub-agent batches within a phase step.** Files may be in transient state. All linter/LSP checks happen AFTER the phase step is complete. -- **Every file with a mapped error MUST be processed.** Skipping due to volume is a failure mode. - -### Per-phase skill dispatch checklist +- **Foreground mode only.** Launch all sub-agents for a phase step in a SINGLE message — blocks until all return. +- **Cap ~8 concurrent per message.** Batch sequentially if more — do NOT stop after one batch. +- **Group related files** (controller + its XML view) into one sub-agent. +- **No validation between batches within a phase step.** Checks happen AFTER phase step completes. +- **Every file with a mapped error MUST be processed.** -Quick reference for which skills get dispatched in each phase. The "Rule ID to Skill Mapping" table above is authoritative — this is a convenience summary. +### Per-phase skill dispatch -| Phase | Skills dispatched | -|-------|-------------------| +| Phase | Skills | +|-------|--------| | 1 | `modernize-test-starter` | | 2 | `fix-manifest-json`, `fix-component-async` | | 3 (parallel) | `fix-js-globals`, `fix-pseudo-modules`, `fix-xml-globals` | @@ -629,34 +296,29 @@ Quick reference for which skills get dispatched in each phase. The "Rule ID to S ## Documentation Phase (after Phase 5) -Create `MODERNIZATION-ISSUES.md` (unfixable errors) and `MODERNIZATION-REPORT.md` (final summary) using the templates in `references/documentation-templates.md`. +Create `MODERNIZATION-ISSUES.md` and `MODERNIZATION-REPORT.md` using templates in `references/documentation-templates.md`. Commit: `docs: add modernization report and issues` ## Context Management -This workflow touches 17+ skill files totaling ~8,000 lines. The main agent's context cannot hold them all. Strategy: **the main agent never reads child skill files — only sub-agents do.** +The main agent never reads child skill files — only sub-agents do. -1. **Never read child skills.** The routing table tells you which skill to assign. The sub-agent prompt tells each sub-agent to read its own skill. Trust the routing. -2. **Parse and discard linter output.** Parse into `[{file, line, rule, message}]`, optionally persist to `/tmp/ui5-modernization-state.json`, then drop the raw text. Give each sub-agent only its filtered errors. -3. **Compress sub-agent results.** Keep: count of errors fixed, unfixable errors (for MODERNIZATION-ISSUES.md), files modified (for the commit). Discard narrative. -4. **Prioritize completion over documentation.** If context runs low after Phase 4, always finish Phase 5 (CSP). Skip or minimize the documentation phase — the commit history already records what changed. +1. **Never read child skills.** Routing table tells you which skill; sub-agent reads its own. +2. **Parse and discard linter output.** Give each sub-agent only its filtered errors. +3. **Compress sub-agent results.** Keep: count fixed, unfixable errors, files modified. +4. **Prioritize completion.** If context runs low, finish Phase 5. Skip documentation if needed. ## Error Handling -If a fix attempt **genuinely fails** (not "there are too many"): -1. Log the error. -2. Add to MODERNIZATION-ISSUES.md. -3. Continue with the next error — do not stop the workflow. +If a fix genuinely fails: log, add to MODERNIZATION-ISSUES.md, continue with next error. ## Completion Checklist -Before the documentation phase: - - [ ] User picked verification mode in Phase 0; agent did not re-ask -- [ ] Each of phases 1–5 has a commit (or was skipped because no changes applied) -- [ ] Verification gate ran after every phase per the chosen mode -- [ ] Phase 3 ran ALL three steps (parallel batch + blind-spots + cycles), even if linter showed 0 errors after the batch -- [ ] Sub-agents launched in foreground, single-message batches; no validation mid-phase +- [ ] Each of phases 1–5 has a commit (or was skipped — no changes) +- [ ] Verification gate ran after every phase per chosen mode +- [ ] Phase 3 ran ALL three steps (parallel + blind-spots + cycles), even if linter showed 0 after batch +- [ ] Sub-agents launched in foreground, single-message batches - [ ] Files staged per-phase (no `git add -A`) - [ ] MODERNIZATION-ISSUES.md contains only genuinely unfixable errors