From 04f7ae327c4e9898511068c451d6738f17088bbe Mon Sep 17 00:00:00 2001 From: Peter Muessig Date: Wed, 9 Sep 2026 15:15:52 +0200 Subject: [PATCH 1/3] feat(runtime): log JSX view-construction errors via sap/base/Log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when a JSX view's createContent() threw (sync or async), the error propagated as a rejected Promise but was never written to the UI5 Log. In the showcase it was only rendered as a text control, invisible to log-scraping tooling and sap-ui-log-level debugging. The fix patches View.prototype.onControllerConnected (not createContent, which is bypassed by subclass dynamic dispatch) to install a per-instance wrapper on this.createContent before calling the original method. The wrapper opens the JSX scope (withScope), wraps the subclass call in try/catch plus .catch() for async, calls Log.error on failure, then re-throws — preserving all existing propagation behaviour. The instance own-property is deleted in a finally block so the prototype chain is fully restored after each view construction. ExploreSample.tsx also adds Log.error calls in both its catch handlers (demo-view load failure and docs-load failure) so the exact user-facing message is traceable in the log. Two QUnit regression tests verify both the sync-throw and async-reject paths: Log.error is called exactly once with the right message and component id, and View.create still rejects. Also brings runtime/forwarded-aggregation.qunit.ts from the fix/forwarded-aggregation-issue-9 branch (was missing on main). --- .changeset/log-view-errors.md | 19 +++ .../webapp/view/ExploreSample.tsx | 11 ++ .../src/runtime/installViewScopeBridge.ts | 129 +++++++++++----- .../runtime/forwarded-aggregation.qunit.ts | 50 ++++++ .../qunit/runtime/view-error-logging.qunit.ts | 144 ++++++++++++++++++ .../jsx-runtime/test/qunit/testsuite.qunit.ts | 2 + 6 files changed, 315 insertions(+), 40 deletions(-) create mode 100644 .changeset/log-view-errors.md create mode 100644 packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts create mode 100644 packages/jsx-runtime/test/qunit/runtime/view-error-logging.qunit.ts diff --git a/.changeset/log-view-errors.md b/.changeset/log-view-errors.md new file mode 100644 index 0000000..b041c71 --- /dev/null +++ b/.changeset/log-view-errors.md @@ -0,0 +1,19 @@ +--- +"@ui5-community/jsx-runtime": patch +--- + +JSX view-construction errors are now logged via `sap/base/Log` in addition +to propagating. Previously, when `createContent()` threw (sync or async), +the error was only visible as a rejected Promise and, in the showcase, as +a text message rendered into the DOM. It was never written to the UI5 Log, +making it invisible to log-scraping tooling and `sap-ui-log-level` debugging. + +`installViewScopeBridge` now wraps the `createContent()` call with a +try/catch (sync) and a `.catch` on the returned Promise (async). On failure +it calls `Log.error` with the full error message, the stack trace, and the +component id `"ui5.community.jsx.runtime"`, then re-throws / re-rejects so +existing propagation behaviour is unchanged. + +The showcase's `ExploreSample` view also adds `Log.error` calls in both its +demo-view and docs-load catch handlers so the exact user-facing message +appears in the log as well. diff --git a/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx b/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx index acf2d79..03ae71f 100644 --- a/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx +++ b/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx @@ -10,6 +10,7 @@ import HBox from "sap/m/HBox"; import Title from "sap/m/Title"; import Text from "sap/m/Text"; import MessageStrip from "sap/m/MessageStrip"; +import Log from "sap/base/Log"; import * as Prism from "prismjs"; import CodeBlock from "../control/CodeBlock"; @@ -324,6 +325,11 @@ export default class ExploreSample extends View { }) .catch((error: unknown) => { if (this.currentSampleId !== sampleId) return; + Log.error( + `Could not load docs/samples/${sampleId}.md: ${String(error)}`, + error instanceof Error ? (error.stack ?? "") : "", + "ui5.community.jsx.showcase.view.ExploreSample" + ); this.description.setContent( `
` + `

Could not load docs/samples/${escapeHtml(sampleId)}.md: ${escapeHtml(String(error))}

` + @@ -350,6 +356,11 @@ export default class ExploreSample extends View { this.demoSlot.removeAllItems(); this.demoSlot.addItem(demoView); }).catch((error: unknown) => { + Log.error( + `Failed to load ${reg.viewName}: ${String(error)}`, + error instanceof Error ? (error.stack ?? "") : "", + "ui5.community.jsx.showcase.view.ExploreSample" + ); this.demoSlot.removeAllItems(); this.demoSlot.addItem(new Text({ text: `Failed to load ${reg.viewName}: ${String(error)}` diff --git a/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts b/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts index 5056f62..a38f663 100644 --- a/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts +++ b/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts @@ -1,5 +1,6 @@ import View from "sap/ui/core/mvc/View"; import type Control from "sap/ui/core/Control"; +import Log from "sap/base/Log"; import { withScope } from "./scope"; /** @@ -15,36 +16,44 @@ import { withScope } from "./scope"; * * The clean way to expose that context is to wrap the * subclass's `createContent()` in a `withScope({ view: this }, - * …)`. Doing that at every JSX-view call site is noise every - * consumer would repeat. Instead this module installs a - * one-time prototype patch on `sap.ui.core.mvc.View.prototype.createContent` - * that transparently opens the scope for the duration of the - * subclass's own `createContent()` call. + * …)`. This module installs a one-time prototype patch on + * `View.prototype.onControllerConnected` that does exactly that: + * before delegating to the original `onControllerConnected`, it + * installs a per-instance wrapper on `this.createContent` that + * opens the scope and catches errors, then cleans up via `finally`. * - * ## Why patch the base prototype + * ## Why patch `onControllerConnected`, not `createContent` * - * `sap.ui.core.mvc.View`'s design invites subclasses to override - * `createContent()`. XMLView, JSONView, TypedView, JSView all - * do exactly that, none of them calls `super.createContent()`, - * and the base method is a no-op that returns `null`. Patching - * the *base* prototype's `createContent` catches every subclass - * override transparently: when a JSX view's own - * `createContent()` runs, it does so with `this === theView` - * *and* under an active JSX scope that carries the view. XMLView - * / JSONView still work, they don't call `jsx()`, so the added - * scope is a no-op for them. + * UI5's `onControllerConnected` calls `this.createContent(e)`, + * which dynamically dispatches to the **subclass** prototype + * (e.g. `MyJsxView.prototype.createContent`). Patching + * `View.prototype.createContent` only intercepts code that + * explicitly calls `super.createContent()` — no View subclass + * ever does that (the base method is a no-op returning `null`). + * + * By patching `onControllerConnected` instead, we install a + * per-instance own-property on `this.createContent` *before* + * the original `onControllerConnected` body runs. Own-properties + * shadow prototype properties, so `this.createContent(...)` in + * `runWithPreprocessors` routes through our wrapper regardless + * of which subclass is in play. + * + * XMLView / JSONView / TypedView override `createContent()` and + * don't call `jsx()`, so the added `withScope` is a no-op for + * them. Error-logging is equally transparent — only JSX views + * are likely to throw novel errors, and the log entry will + * clearly identify the view by id. * * ## The idempotence guard * * `installed` prevents double-wrapping if this module is loaded * twice (e.g. via two different resource-root mappings, or from - * a test harness that reloads the runtime). Double-wrapping - * wouldn't be catastrophic (the inner `withScope` merges its - * partial on top of the parent scope), but it would obscure the - * ambient-state chain and every retry would pay a stack frame. + * a test harness that reloads the runtime). * * @namespace ui5.community.jsx.runtime.jsx-runtime */ +const COMPONENT = "ui5.community.jsx.runtime"; + let installed = false; /** @@ -56,24 +65,64 @@ let installed = false; export function installViewScopeBridge(): void { if (installed) return; installed = true; - const original = View.prototype.createContent as ( - this: View - ) => Control | Control[] | Promise; - View.prototype.createContent = function patchedCreateContent( - this: View - ): Control | Control[] | Promise { - // The `view: this` slot on `Scope` is what `jsx()` reads - // when deciding whether to auto-prefix a child control's - // `id`. The scope is popped on the callback's synchronous - // return; async `createContent()` overrides (those that - // return a Promise) also get the view in scope only for - // the synchronous body preceding the first `await`, the - // hot path for auto-prefixing (control construction inside - // a JSX expression before an `await`) is fully covered. - // - // XMLView / JSONView / TypedView override `createContent()` - // without ever calling `jsx()`, so the ambient scope is - // unused for them. Zero cost. - return withScope({ view: this }, () => original.apply(this, [])); - } as typeof View.prototype.createContent; + + // `onControllerConnected` is an internal UI5 method not exposed in the + // public TypeScript typings — use `any` casts to access/patch it. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const viewProto = View.prototype as any; + type CreateContentFn = (this: View) => Control | Control[] | Promise; + type OnControllerConnectedFn = (this: View, controller: unknown, settings?: unknown) => unknown; + + const originalOnControllerConnected = viewProto.onControllerConnected as OnControllerConnectedFn; + + viewProto.onControllerConnected = function patchedOnControllerConnected( + this: View, + controller: unknown, + settings?: unknown + ): unknown { + // Retrieve the subclass's (or base's) createContent *now*, before we + // shadow it on the instance. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const subclassCreateContent = (this as any).createContent as CreateContentFn; + + const viewId = this.getId(); + + const logAndRethrow = (error: unknown): never => { + Log.error( + `createContent() failed for view '${viewId}': ${String(error)}`, + error instanceof Error ? (error.stack ?? "") : "", + COMPONENT + ); + throw error; + }; + + // Install the per-instance wrapper that the `onControllerConnected` + // body will call via `this.createContent(...)`. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (this as any).createContent = function wrappedCreateContent( + this: View + ): Control | Control[] | Promise { + return withScope({ view: this }, () => { + let result: Control | Control[] | Promise; + try { + result = subclassCreateContent.call(this); + } catch (error) { + return logAndRethrow(error); + } + if (result instanceof Promise) { + return result.catch(logAndRethrow); + } + return result; + }); + }; + + try { + return originalOnControllerConnected.call(this, controller, settings); + } finally { + // Always clean up the instance shadow so the prototype chain is + // restored for any subsequent calls on this view instance. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (this as any).createContent; + } + }; } diff --git a/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts b/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts new file mode 100644 index 0000000..46bb402 --- /dev/null +++ b/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts @@ -0,0 +1,50 @@ +/** + * QUnit regression tests for forwarded default aggregations (issue #9). + * + * Controls like `sap.m.Menu` declare their default aggregation (`items`) + * with **aggregation forwarding**: the real target is an internal + * `MenuWrapper` created in `Menu.prototype.init()` and resolved by id at + * add-time. Passing `items` in the single-shot `new Menu({ items })` call + * (our default construct path) makes `applySettings` attempt to resolve + * the wrapper before it is registered, causing a `TypeError`. + * + * The fix detects a forwarded default aggregation via + * `metadata.getAggregation(name)?.forwarding` and defers concrete children + * to a post-construction `addAggregation` call, matching how XMLView fills + * such aggregations. + * + * These tests deliberately create `Menu` instances **without** an explicit + * `id` so the broken code path is exercised (an explicit id was the + * reporter's workaround that masked the bug). + */ +import { jsx } from "ui5/community/jsx/runtime/jsx-runtime"; +import Menu from "sap/m/Menu"; +import MenuItem from "sap/m/MenuItem"; + +QUnit.module("runtime/forwarded-aggregation"); + +QUnit.test("Menu with multiple children (no id) — items populated correctly", (assert) => { + const menu = jsx(Menu, { + children: [ + jsx(MenuItem, { text: "One" }), + jsx(MenuItem, { text: "Two" }), + ] + }) as Menu; + + const items = menu.getItems(); + assert.strictEqual(items.length, 2, "two items forwarded into the Menu"); + assert.strictEqual((items[0] as MenuItem).getText(), "One", "first item text correct"); + assert.strictEqual((items[1] as MenuItem).getText(), "Two", "second item text correct"); + menu.destroy(); +}); + +QUnit.test("Menu with a single child (no id) — item populated correctly", (assert) => { + const menu = jsx(Menu, { + children: jsx(MenuItem, { text: "Only" }), + }) as Menu; + + const items = menu.getItems(); + assert.strictEqual(items.length, 1, "one item forwarded into the Menu"); + assert.strictEqual((items[0] as MenuItem).getText(), "Only", "item text correct"); + menu.destroy(); +}); diff --git a/packages/jsx-runtime/test/qunit/runtime/view-error-logging.qunit.ts b/packages/jsx-runtime/test/qunit/runtime/view-error-logging.qunit.ts new file mode 100644 index 0000000..358d0da --- /dev/null +++ b/packages/jsx-runtime/test/qunit/runtime/view-error-logging.qunit.ts @@ -0,0 +1,144 @@ +/** + * QUnit regression tests: JSX view-construction errors are written to the + * UI5 Log (not just propagated as a rejected Promise). + * + * `installViewScopeBridge` patches `View.prototype.createContent` so that + * any thrown error — whether synchronous or from an async `createContent` + * that returns a rejected Promise — is: + * 1. Logged via `Log.error` with a message that includes the view id and + * the original error text. + * 2. Re-thrown / re-rejected, so callers (e.g. `View.create`) still + * receive the rejection unchanged. + * + * The tests trigger the two error paths by registering tiny inline View + * modules and loading them via `View.create`. `Log.error` is swapped out + * for a recorder for the duration of each test. UI5's View lifecycle + * produces a secondary unhandled rejection after the first one propagates; + * we intercept it with a window-level `unhandledrejection` handler so + * QUnit doesn't count it as a test failure. + */ +import View from "sap/ui/core/mvc/View"; +import Log from "sap/base/Log"; +import { installViewScopeBridge } from "ui5/community/jsx/runtime/runtime/installViewScopeBridge"; + +// Ensure the bridge patch is applied before the first test runs. +installViewScopeBridge(); + +/** Replace `Log.error` with a call recorder for one test. */ +function stubLogError(): { + calls: Array<[string, string, string]>; + restore: () => void; +} { + const calls: Array<[string, string, string]> = []; + const original = Log.error.bind(Log); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (Log as any).error = (msg: string, detail: string, component: string) => { + calls.push([msg, detail, component]); + }; + return { + calls, + restore() { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (Log as any).error = original; + } + }; +} + +/** Suppress unhandled rejections for the duration of a callback. */ +function withRejectionSuppression(fn: () => Promise): Promise { + const handler = (e: PromiseRejectionEvent) => e.preventDefault(); + window.addEventListener("unhandledrejection", handler); + return fn().finally(() => window.removeEventListener("unhandledrejection", handler)); +} + +QUnit.module("runtime/view-error-logging"); + +// --------------------------------------------------------------------------- +// Sync-throw variant +// --------------------------------------------------------------------------- + +QUnit.test("sync throw in createContent() — Log.error called once, rejection preserved", (assert) => { + const done = assert.async(); + return withRejectionSuppression(async () => { + const stub = stubLogError(); + let caught: unknown; + try { + sap.ui.define( + "test/view/ThrowingView", + ["sap/ui/core/mvc/View"], + function (BaseView: typeof View) { + return BaseView.extend("test.view.ThrowingView", { + createContent() { + throw new TypeError("deliberate sync error"); + } + }); + } + ); + + try { + await View.create({ viewName: "module:test/view/ThrowingView" }); + } catch (err) { + caught = err; + } + + assert.ok(caught != null, "View.create rejected on createContent() failure"); + assert.strictEqual(stub.calls.length, 1, "Log.error called exactly once"); + if (stub.calls.length >= 1) { + const [msg, , component] = stub.calls[0]; + assert.ok( + msg.includes("deliberate sync error"), + `log message contains original error text: "${msg}"` + ); + assert.strictEqual(component, "ui5.community.jsx.runtime", "component id correct"); + } + } finally { + stub.restore(); + done(); + } + }); +}); + +// --------------------------------------------------------------------------- +// Async-reject variant +// --------------------------------------------------------------------------- + +QUnit.test("async rejected createContent() — Log.error called once, rejection preserved", (assert) => { + const done = assert.async(); + return withRejectionSuppression(async () => { + const stub = stubLogError(); + let caught: unknown; + try { + sap.ui.define( + "test/view/AsyncThrowingView", + ["sap/ui/core/mvc/View"], + function (BaseView: typeof View) { + return BaseView.extend("test.view.AsyncThrowingView", { + createContent() { + return Promise.reject(new RangeError("deliberate async error")); + } + }); + } + ); + + try { + await View.create({ viewName: "module:test/view/AsyncThrowingView" }); + } catch (err) { + caught = err; + } + + assert.ok(caught != null, "View.create rejected on async createContent() failure"); + assert.strictEqual(stub.calls.length, 1, "Log.error called exactly once"); + if (stub.calls.length >= 1) { + const [msg, , component] = stub.calls[0]; + assert.ok( + msg.includes("deliberate async error"), + `log message contains original error text: "${msg}"` + ); + assert.strictEqual(component, "ui5.community.jsx.runtime", "component id correct"); + } + } finally { + stub.restore(); + done(); + } + }); +}); diff --git a/packages/jsx-runtime/test/qunit/testsuite.qunit.ts b/packages/jsx-runtime/test/qunit/testsuite.qunit.ts index 9b024cf..e1cfbbb 100644 --- a/packages/jsx-runtime/test/qunit/testsuite.qunit.ts +++ b/packages/jsx-runtime/test/qunit/testsuite.qunit.ts @@ -34,6 +34,8 @@ export default { "runtime/property-appliers": { title: "runtime: PropertyApplier ordering" }, "runtime/auto-prefix-id": { title: "runtime: auto id prefix bridge" }, "runtime/string-aggregation-binding": { title: "runtime: string aggregation binding + template" }, + "runtime/forwarded-aggregation": { title: "runtime: forwarded default aggregation (Menu#items)" }, + "runtime/view-error-logging": { title: "runtime: view createContent() errors logged via Log.error" }, "plugins/switch": { title: "plugins: / / " } } }; From a9c0438454f3ff41a74170bb03079347781e1bd2 Mon Sep 17 00:00:00 2001 From: Peter Muessig Date: Wed, 9 Sep 2026 10:50:53 +0200 Subject: [PATCH 2/3] fix(runtime): defer forwarded default-aggregation children to post-construction Controls like sap.m.Menu declare their default aggregation (items) as forwarded to an internal wrapper control created in init() and resolved by id at add-time. Passing children via new Type({items}) made applySettings attempt the lookup before the wrapper was registered, crashing with a TypeError (issue #9). The runtime now detects a forwarded default aggregation via metadata.getAggregation(name)?.forwarding and defers concrete children to a post-construction addAggregation loop, matching XMLView behaviour. String-binding and template children are not affected. - Add getAggregation() to the ControlMetadata structural type (plugin.ts) - Patch the concrete-children branch in runtime.ts - Add QUnit regression test (forwarded-aggregation.qunit.ts) - Add ForwardedMenu showcase example (button menu + List contextMenu) --- .changeset/fix-forwarded-aggregation.md | 14 +++ packages/jsx-runtime-showcase/samples.json | 9 ++ .../webapp/controller/Explorer.controller.ts | 3 +- .../webapp/docs/samples/forwarded-menu.md | 67 +++++++++++ .../webapp/view/ExploreSample.tsx | 6 + .../webapp/view/showcases/ForwardedMenu.tsx | 112 ++++++++++++++++++ packages/jsx-runtime/src/runtime/plugin.ts | 11 ++ packages/jsx-runtime/src/runtime/runtime.ts | 29 ++++- 8 files changed, 249 insertions(+), 2 deletions(-) create mode 100644 .changeset/fix-forwarded-aggregation.md create mode 100644 packages/jsx-runtime-showcase/webapp/docs/samples/forwarded-menu.md create mode 100644 packages/jsx-runtime-showcase/webapp/view/showcases/ForwardedMenu.tsx diff --git a/.changeset/fix-forwarded-aggregation.md b/.changeset/fix-forwarded-aggregation.md new file mode 100644 index 0000000..27a7d16 --- /dev/null +++ b/.changeset/fix-forwarded-aggregation.md @@ -0,0 +1,14 @@ +--- +"@ui5-community/jsx-runtime": patch +--- + +Fix: forwarded default aggregations (e.g. `sap.m.Menu#items`) now work in +JSX without an explicit `id` on the control. + +Controls that forward their default aggregation to an internal child +(resolved by id in `init()`) previously crashed with a `TypeError` when +JSX children were passed in the single-shot `new Type(settings)` call. +The runtime now detects a forwarded default aggregation via +`metadata.getAggregation(name)?.forwarding` and defers concrete children +to a post-construction `addAggregation` call, matching how XMLView fills +such aggregations. Closes #9. diff --git a/packages/jsx-runtime-showcase/samples.json b/packages/jsx-runtime-showcase/samples.json index 509464f..c51f510 100644 --- a/packages/jsx-runtime-showcase/samples.json +++ b/packages/jsx-runtime-showcase/samples.json @@ -154,6 +154,15 @@ "viewName": "ui5.community.jsx.showcase.view.showcases.TableBound", "sourcePath": "webapp/view/showcases/TableBound.tsx", "docPath": "webapp/docs/samples/table-bound.md" + }, + { + "id": "forwarded-menu", + "title": "Forwarded aggregation (Menu)", + "chapter": 18, + "concepts": ["forwarded aggregation", "sap.m.Menu", "contextMenu", "no explicit id"], + "viewName": "ui5.community.jsx.showcase.view.showcases.ForwardedMenu", + "sourcePath": "webapp/view/showcases/ForwardedMenu.tsx", + "docPath": "webapp/docs/samples/forwarded-menu.md" } ] } diff --git a/packages/jsx-runtime-showcase/webapp/controller/Explorer.controller.ts b/packages/jsx-runtime-showcase/webapp/controller/Explorer.controller.ts index b89735c..41ec6bd 100644 --- a/packages/jsx-runtime-showcase/webapp/controller/Explorer.controller.ts +++ b/packages/jsx-runtime-showcase/webapp/controller/Explorer.controller.ts @@ -114,7 +114,8 @@ export default class Explorer extends Controller { { key: "explore.controller-as-file", title: "Controller as separate file", icon: "sap-icon://source-code", route: "exploreSample", arg: "controller-as-file" }, { key: "explore.structural", title: "Structural , ", icon: "sap-icon://tree", route: "exploreSample", arg: "structural" }, { key: "explore.type-coercion", title: "Property type coercion", icon: "sap-icon://alert", route: "exploreSample", arg: "type-coercion" }, - { key: "explore.custom-data-key", title: "CustomData & the key prop", icon: "sap-icon://key-user-settings", route: "exploreSample", arg: "custom-data-key" } + { key: "explore.custom-data-key", title: "CustomData & the key prop", icon: "sap-icon://key-user-settings", route: "exploreSample", arg: "custom-data-key" }, + { key: "explore.forwarded-menu", title: "Forwarded aggregation (Menu)", icon: "sap-icon://menu2", route: "exploreSample", arg: "forwarded-menu" } ]}, { key: "explore.g.plugins", title: "Plugins / Extend", icon: "sap-icon://puzzle", children: [ { key: "explore.switch-plugin", title: "Plugin ", icon: "sap-icon://switch-classes", route: "exploreSample", arg: "switch-plugin" } diff --git a/packages/jsx-runtime-showcase/webapp/docs/samples/forwarded-menu.md b/packages/jsx-runtime-showcase/webapp/docs/samples/forwarded-menu.md new file mode 100644 index 0000000..ecd1c11 --- /dev/null +++ b/packages/jsx-runtime-showcase/webapp/docs/samples/forwarded-menu.md @@ -0,0 +1,67 @@ +Demonstrates `sap.m.Menu` rendered via JSX — both as a standalone +button menu and as a `List`'s `contextMenu` — **without** an explicit +`id` on the `Menu`. + +## Why no explicit id? + +`sap.m.Menu` declares its default `items` aggregation as **forwarded**: +the real backing store is an internal `MenuWrapper` control that +`Menu.prototype.init()` creates and registers under the id +`-menuWrapper`. When `Menu` items ride along in the single-shot +`new Menu({ items })` constructor call, UI5's `applySettings` tries to +resolve the wrapper by id *before* it is registered — crashing with: + +``` +TypeError: Cannot read properties of undefined (reading 'addItem') +``` + +The reporter's workaround (giving the `Menu` an explicit `id`) happened +to make the wrapper's auto-generated id stable enough that the lookup +succeeded. The proper fix is to defer the items to a post-construction +`addAggregation` call, matching how `XMLView` populates forwarded +aggregations. + +## What the fix does + +The runtime detects a forwarded default aggregation via +`metadata.getAggregation(name)?.forwarding` and, when the aggregation is +forwarded, skips the `settings` route and uses a post-construction loop: + +```tsx +// After construction, the MenuWrapper is already registered +menu.addAggregation("items", child); +``` + +Normal (non-forwarded) aggregations are not affected. + +## Two patterns in this sample + +```tsx +// Pattern 1 — standalone Menu (forwarded default-aggregation children) +const menu = ( + + + + +) as Menu; +// ...later +menu.openBy(button); + +// Pattern 2 — List contextMenu (aggregation prop, items also forwarded) + + + + + } + items={{ path: "/fruits" }} +> + + +``` + +Neither `Menu` carries an explicit `id`. + +See also: [aggregations](#/learn/aggregations) — how the runtime +maps JSX children to UI5 aggregations. diff --git a/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx b/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx index 03ae71f..356bc54 100644 --- a/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx +++ b/packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx @@ -155,6 +155,12 @@ const REGISTRY: Record` children in JSX would crash with + * a `TypeError` unless the `Menu` was given an explicit `id` (the + * reporter's workaround). Both menus on this page are intentionally + * created **without** an explicit `id` to exercise the fixed code path. + * + * Two patterns shown side-by-side: + * + * 1. **Standalone menu** — a Button whose `press` calls `menu.openBy()` on + * a `` built from `` JSX children (the default- + * aggregation / forwarded path that was previously broken). + * 2. **List context menu** — a `` with `contextMenu={…}` + * (aggregation-prop form). UI5 opens it automatically on right-click / + * long-press; the `items` forwarding still applies inside the `Menu`. + * + * @namespace ui5.community.jsx.showcase.view.showcases + */ + +type Fruit = { name: string; emoji: string }; + +export default class ForwardedMenu extends View { + private readonly model = new JSONModel({ + fruits: [ + { name: "Apple", emoji: "🍎" }, + { name: "Banana", emoji: "🍌" }, + { name: "Cherry", emoji: "🍒" } + ] satisfies Fruit[] + }); + + /** Opened by the "Open menu" button. Not placed in the layout tree. */ + private readonly actionMenu: Menu = ( + + + + + + ) as Menu; + + getAutoPrefixId(): boolean { + return true; + } + + createContent(): Control { + this.setModel(this.model); + // Tie the standalone Menu's lifecycle to this view so it is + // destroyed with it (same as placing it in a view dependent). + this.addDependent(this.actionMenu); + + // The context menu is set directly on the List via the `contextMenu` + // aggregation prop. UI5 manages its lifecycle with the List. + const contextMenu = ( + + + + + ) as Menu; + + return ( + + {/* ── Section 1: standalone button menu ──────────────── */} + + <Text + text="Press the button to open a Menu whose items are declared as JSX children. No explicit id on the Menu." + class="sapUiSmallMarginBottom" + /> + <HBox> + <Button + text="Open menu" + icon="sap-icon://menu2" + press={this.onOpenMenu.bind(this)} + /> + </HBox> + + {/* ── Section 2: List context menu ───────────────────── */} + <Title text="List with contextMenu" level="H4" class="sapUiSmallMarginTop sapUiSmallMarginBottom" /> + <Text + text="Right-click (or long-press on mobile) a row to open the context Menu. The Menu's items are also JSX children — no explicit id." + class="sapUiSmallMarginBottom" + /> + <List + headerText="Fruits" + contextMenu={contextMenu} + items={{ path: "/fruits" }} + > + <StandardListItem title="{emoji} {name}" /> + </List> + </VBox> + ); + } + + onOpenMenu(event: { getSource(): Button }): void { + this.actionMenu.openBy(event.getSource()); + } +} diff --git a/packages/jsx-runtime/src/runtime/plugin.ts b/packages/jsx-runtime/src/runtime/plugin.ts index bf2489d..fedf536 100644 --- a/packages/jsx-runtime/src/runtime/plugin.ts +++ b/packages/jsx-runtime/src/runtime/plugin.ts @@ -43,6 +43,17 @@ export type ControlMetadata = { * `ManagedObjectMetadata.hasAggregation`. */ hasAggregation(name: string): boolean; + /** + * Returns the aggregation descriptor for `name`, or `undefined` when no + * such aggregation is declared. The returned object's public `forwarding` + * field is set when the aggregation is forwarded to an internal child + * control (e.g. `sap.m.Menu#items` → its `-menuWrapper`). The runtime + * uses this to detect a forwarded *default* aggregation and defer its + * concrete children to a post-construction `addAggregation` call, since + * the forwarding target isn't resolvable during the single-shot + * `new type(settings)`. Backed by `ManagedObjectMetadata.getAggregation`. + */ + getAggregation(name: string): { forwarding?: unknown } | undefined; /** * Returns the property descriptor (containing `type: string`, the UI5 * type name like `"int"`, `"boolean"`, `"sap.m.ButtonType"`) or diff --git a/packages/jsx-runtime/src/runtime/runtime.ts b/packages/jsx-runtime/src/runtime/runtime.ts index dd29eec..562b7de 100644 --- a/packages/jsx-runtime/src/runtime/runtime.ts +++ b/packages/jsx-runtime/src/runtime/runtime.ts @@ -318,6 +318,17 @@ export function jsx<T extends ManagedObject>( // and `<Fragment>` inline. Plugins can add `<Switch>`, etc. const processed = processChildren(children, settings, defaultAggregation); const arr = flattenChildren(processed); + // Detect a forwarded aggregation early (e.g. sap.m.Menu#items). The + // forwarding target (an internal wrapper control) is created inside + // `init()` and resolved by id at add-time. When concrete children + // ride along in `new type(settings)`, `applySettings` tries to + // resolve the target before it is registered → TypeError. We defer + // those children to a post-construction `addAggregation` call (see + // the `else if (arr.length > 0)` branch below), matching the + // XMLView fill order. Bound forwarded aggregations (string-binding / + // template path) are not affected — only the concrete-children case. + const defaultAggForwarded = + metadata.getAggregation(defaultAggregation)?.forwarding !== undefined; let existing = settings[defaultAggregation] as | { path?: unknown; template?: unknown } | undefined; @@ -367,7 +378,23 @@ export function jsx<T extends ManagedObject>( existing.template = arr.length === 1 ? arr[0] : arr; } } else if (arr.length > 0) { - settings[defaultAggregation] = arr.length === 1 ? arr[0] : arr; + if (defaultAggForwarded) { + // Forwarded aggregations (e.g. sap.m.Menu#items) route to an + // internal target that is created in `init()` but only + // resolvable by id *after* construction. Passing children via + // `settings` makes `applySettings` attempt the lookup too + // early → TypeError. Fill them after construction via + // `addAggregation`, matching how XMLView populates them. + const forwardedChildren = arr; + post((instance) => { + for (const child of forwardedChildren) { + (instance as { addAggregation?: (name: string, oObject: unknown) => void }) + .addAggregation?.(defaultAggregation, child); + } + }); + } else { + settings[defaultAggregation] = arr.length === 1 ? arr[0] : arr; + } } } } From 62ef3c23da9ca66f0a0c314bd355d3842029a70a Mon Sep 17 00:00:00 2001 From: Peter Muessig <peter.muessig@sap.com> Date: Wed, 9 Sep 2026 22:38:10 +0200 Subject: [PATCH 3/3] fix(runtime): skip generated ids in auto-prefix-id + createId bridge (closes #9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UI5's View.onControllerConnected uses runWithPreprocessors to apply view.createId as the id-preprocessor for all ManagedObjects constructed inside createContent(). Controls like sap.m.Menu create internal helpers (MenuWrapper) whose ids are derived from an auto-generated parent id (e.g. "__menu0-menuWrapper"). With getAutoPrefixId() === true, these derived- generated ids were being run through view.createId(), making them unresolvable when the aggregation forwarder later looked them up by id. The fix is in two layers: 1. installViewScopeBridge: shadows view.createId with a guard that calls ManagedObjectMetadata.isGeneratedId and returns the id unchanged when it matches the generated-id pattern (starts with or contains the UID prefix, default "__"). This guards the runWithPreprocessors path — internal control ids stay stable regardless of the view's autoPrefixId setting. 2. runtime.ts: adds the same isGeneratedId guard to the explicit view.createId() call in the JSX auto-prefix-id block so JSX-authored id props are only prefixed when they are statically supplied by the app developer. Also reverts the incorrect "forwarded aggregation" deferral (defaultAggForwarded + post-construction addAggregation) that was a symptom-level workaround for the same root cause. --- .changeset/fix-forwarded-aggregation.md | 20 +-- .../src/runtime/installViewScopeBridge.ts | 24 ++- packages/jsx-runtime/src/runtime/plugin.ts | 11 -- packages/jsx-runtime/src/runtime/runtime.ts | 32 +--- .../runtime/forwarded-aggregation.qunit.ts | 146 ++++++++++++------ 5 files changed, 140 insertions(+), 93 deletions(-) diff --git a/.changeset/fix-forwarded-aggregation.md b/.changeset/fix-forwarded-aggregation.md index 27a7d16..d7039d9 100644 --- a/.changeset/fix-forwarded-aggregation.md +++ b/.changeset/fix-forwarded-aggregation.md @@ -2,13 +2,15 @@ "@ui5-community/jsx-runtime": patch --- -Fix: forwarded default aggregations (e.g. `sap.m.Menu#items`) now work in -JSX without an explicit `id` on the control. +Fix: auto-prefix-id no longer prefixes UI5-generated ids (closes #9). -Controls that forward their default aggregation to an internal child -(resolved by id in `init()`) previously crashed with a `TypeError` when -JSX children were passed in the single-shot `new Type(settings)` call. -The runtime now detects a forwarded default aggregation via -`metadata.getAggregation(name)?.forwarding` and defers concrete children -to a post-construction `addAggregation` call, matching how XMLView fills -such aggregations. Closes #9. +Controls like `sap.m.Menu` create internal helper controls (e.g. a +`MenuWrapper`) whose ids are generated by UI5 (matching +`ManagedObjectMetadata.isGeneratedId`). Previously, when JSX constructed +such controls inside a view with `getAutoPrefixId() === true`, those +generated ids were passed through `view.createId()`, making the internal +control unresolvable at add-time and causing a `TypeError`. + +The runtime now guards the `createId()` call with +`!ManagedObjectMetadata.isGeneratedId(rawId)` so only author-supplied +(static) ids are prefixed. Generated ids pass through untouched. Closes #9. diff --git a/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts b/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts index a38f663..c4b81c7 100644 --- a/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts +++ b/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts @@ -1,6 +1,7 @@ import View from "sap/ui/core/mvc/View"; import type Control from "sap/ui/core/Control"; import Log from "sap/base/Log"; +import ManagedObjectMetadata from "sap/ui/base/ManagedObjectMetadata"; import { withScope } from "./scope"; /** @@ -116,13 +117,34 @@ export function installViewScopeBridge(): void { }); }; + // Guard `View.createId` against generated ids for the duration of + // `onControllerConnected`. UI5's `runWithPreprocessors` sets + // `View.createId` as the id-preprocessor for ALL ManagedObjects + // constructed inside `createContent()`, including internal controls + // like Menu's `MenuWrapper` whose ids are derived from a generated + // parent id. Without this guard, those generated-derived ids get + // view-prefixed, making them unresolvable when the aggregation + // forwarder later looks them up. The instance shadow is removed in + // the `finally` block to restore the normal prototype chain. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const originalCreateId = (this as any).createId as (sId: string) => string; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (this as any).createId = function guardedCreateId(this: View, sId: string): string { + if (ManagedObjectMetadata.isGeneratedId(sId)) { + return sId; + } + return originalCreateId.call(this, sId); + }; + try { return originalOnControllerConnected.call(this, controller, settings); } finally { - // Always clean up the instance shadow so the prototype chain is + // Always clean up the instance shadows so the prototype chain is // restored for any subsequent calls on this view instance. // eslint-disable-next-line @typescript-eslint/no-explicit-any delete (this as any).createContent; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (this as any).createId; } }; } diff --git a/packages/jsx-runtime/src/runtime/plugin.ts b/packages/jsx-runtime/src/runtime/plugin.ts index fedf536..bf2489d 100644 --- a/packages/jsx-runtime/src/runtime/plugin.ts +++ b/packages/jsx-runtime/src/runtime/plugin.ts @@ -43,17 +43,6 @@ export type ControlMetadata = { * `ManagedObjectMetadata.hasAggregation`. */ hasAggregation(name: string): boolean; - /** - * Returns the aggregation descriptor for `name`, or `undefined` when no - * such aggregation is declared. The returned object's public `forwarding` - * field is set when the aggregation is forwarded to an internal child - * control (e.g. `sap.m.Menu#items` → its `-menuWrapper`). The runtime - * uses this to detect a forwarded *default* aggregation and defer its - * concrete children to a post-construction `addAggregation` call, since - * the forwarding target isn't resolvable during the single-shot - * `new type(settings)`. Backed by `ManagedObjectMetadata.getAggregation`. - */ - getAggregation(name: string): { forwarding?: unknown } | undefined; /** * Returns the property descriptor (containing `type: string`, the UI5 * type name like `"int"`, `"boolean"`, `"sap.m.ButtonType"`) or diff --git a/packages/jsx-runtime/src/runtime/runtime.ts b/packages/jsx-runtime/src/runtime/runtime.ts index 562b7de..39e183b 100644 --- a/packages/jsx-runtime/src/runtime/runtime.ts +++ b/packages/jsx-runtime/src/runtime/runtime.ts @@ -1,4 +1,5 @@ import ManagedObject from "sap/ui/base/ManagedObject"; +import ManagedObjectMetadata from "sap/ui/base/ManagedObjectMetadata"; import DataType from "sap/ui/base/DataType"; import BindingParser from "sap/ui/base/BindingParser"; import View from "sap/ui/core/mvc/View"; @@ -318,17 +319,6 @@ export function jsx<T extends ManagedObject>( // and `<Fragment>` inline. Plugins can add `<Switch>`, etc. const processed = processChildren(children, settings, defaultAggregation); const arr = flattenChildren(processed); - // Detect a forwarded aggregation early (e.g. sap.m.Menu#items). The - // forwarding target (an internal wrapper control) is created inside - // `init()` and resolved by id at add-time. When concrete children - // ride along in `new type(settings)`, `applySettings` tries to - // resolve the target before it is registered → TypeError. We defer - // those children to a post-construction `addAggregation` call (see - // the `else if (arr.length > 0)` branch below), matching the - // XMLView fill order. Bound forwarded aggregations (string-binding / - // template path) are not affected — only the concrete-children case. - const defaultAggForwarded = - metadata.getAggregation(defaultAggregation)?.forwarding !== undefined; let existing = settings[defaultAggregation] as | { path?: unknown; template?: unknown } | undefined; @@ -378,23 +368,7 @@ export function jsx<T extends ManagedObject>( existing.template = arr.length === 1 ? arr[0] : arr; } } else if (arr.length > 0) { - if (defaultAggForwarded) { - // Forwarded aggregations (e.g. sap.m.Menu#items) route to an - // internal target that is created in `init()` but only - // resolvable by id *after* construction. Passing children via - // `settings` makes `applySettings` attempt the lookup too - // early → TypeError. Fill them after construction via - // `addAggregation`, matching how XMLView populates them. - const forwardedChildren = arr; - post((instance) => { - for (const child of forwardedChildren) { - (instance as { addAggregation?: (name: string, oObject: unknown) => void }) - .addAggregation?.(defaultAggregation, child); - } - }); - } else { - settings[defaultAggregation] = arr.length === 1 ? arr[0] : arr; - } + settings[defaultAggregation] = arr.length === 1 ? arr[0] : arr; } } } @@ -429,7 +403,7 @@ export function jsx<T extends ManagedObject>( ) { const rawId = settings.id; const viewId = (activeView as { getId: () => string }).getId(); - if (!rawId.startsWith(`${viewId}--`)) { + if (!rawId.startsWith(`${viewId}--`) && !ManagedObjectMetadata.isGeneratedId(rawId)) { settings.id = (activeView as { createId: (id: string) => string }).createId(rawId); } } diff --git a/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts b/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts index 46bb402..5cdf9f8 100644 --- a/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts +++ b/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts @@ -1,50 +1,110 @@ /** - * QUnit regression tests for forwarded default aggregations (issue #9). - * - * Controls like `sap.m.Menu` declare their default aggregation (`items`) - * with **aggregation forwarding**: the real target is an internal - * `MenuWrapper` created in `Menu.prototype.init()` and resolved by id at - * add-time. Passing `items` in the single-shot `new Menu({ items })` call - * (our default construct path) makes `applySettings` attempt to resolve - * the wrapper before it is registered, causing a `TypeError`. - * - * The fix detects a forwarded default aggregation via - * `metadata.getAggregation(name)?.forwarding` and defers concrete children - * to a post-construction `addAggregation` call, matching how XMLView fills - * such aggregations. - * - * These tests deliberately create `Menu` instances **without** an explicit - * `id` so the broken code path is exercised (an explicit id was the - * reporter's workaround that masked the bug). + * QUnit regression tests for issue #9: the auto-prefix-id feature must not + * prefix UI5-generated ids. */ -import { jsx } from "ui5/community/jsx/runtime/jsx-runtime"; +import View from "sap/ui/core/mvc/View"; import Menu from "sap/m/Menu"; import MenuItem from "sap/m/MenuItem"; +import { jsx, withScope } from "ui5/community/jsx/runtime/jsx-runtime"; QUnit.module("runtime/forwarded-aggregation"); -QUnit.test("Menu with multiple children (no id) — items populated correctly", (assert) => { - const menu = jsx(Menu, { - children: [ - jsx(MenuItem, { text: "One" }), - jsx(MenuItem, { text: "Two" }), - ] - }) as Menu; - - const items = menu.getItems(); - assert.strictEqual(items.length, 2, "two items forwarded into the Menu"); - assert.strictEqual((items[0] as MenuItem).getText(), "One", "first item text correct"); - assert.strictEqual((items[1] as MenuItem).getText(), "Two", "second item text correct"); - menu.destroy(); -}); - -QUnit.test("Menu with a single child (no id) — item populated correctly", (assert) => { - const menu = jsx(Menu, { - children: jsx(MenuItem, { text: "Only" }), - }) as Menu; - - const items = menu.getItems(); - assert.strictEqual(items.length, 1, "one item forwarded into the Menu"); - assert.strictEqual((items[0] as MenuItem).getText(), "Only", "item text correct"); - menu.destroy(); -}); +/** + * Build a fresh anonymous JSX view with getAutoPrefixId() === true. + */ +function makeAutoPrefixView(factory: (view: View) => unknown): View { + const ViewSub = View.extend("qunit.jsx.MenuAutoPfx" + Math.floor(Math.random() * 1e9), { + getAutoPrefixId(): boolean { + return true; + }, + createContent(this: View): unknown { + return factory(this); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any) as unknown as { new (): View }; + return new ViewSub(); +} + +QUnit.test( + "Menu with children inside explicit withScope(view) — no TypeError", + async (assert) => { + const dummyView = makeAutoPrefixView(() => null); + await dummyView.loaded(); + + let caught: unknown; + let menu: Menu | undefined; + try { + withScope({ view: dummyView }, () => { + menu = jsx(Menu, { + children: [ + jsx(MenuItem, { text: "X" }), + jsx(MenuItem, { text: "Y" }), + ] + }) as Menu; + }); + } catch (err) { + caught = err; + } + + assert.ok(caught == null, `no error thrown: ${String(caught)}`); + if (menu) { + const items = menu.getItems(); + assert.strictEqual(items.length, 2, "two items in Menu"); + assert.strictEqual((items[0] as MenuItem).getText(), "X", "first item text"); + assert.strictEqual((items[1] as MenuItem).getText(), "Y", "second item text"); + menu.destroy(); + } + dummyView.destroy(); + } +); + +QUnit.test( + "Menu with children (no explicit id) in autoPrefixId view — no TypeError", + async (assert) => { + let view: View | undefined; + let caught: unknown; + try { + view = makeAutoPrefixView(() => + jsx(Menu, { + children: [ + jsx(MenuItem, { text: "One" }), + jsx(MenuItem, { text: "Two" }), + ] + }) + ); + await view.loaded(); + } catch (err) { + caught = err; + } + + assert.ok(caught == null, `no error thrown: ${String(caught)}`); + if (view) { + const content = view.getContent(); + assert.strictEqual(content.length, 1, "view has one root control (Menu)"); + const menu = content[0] as Menu; + const items = menu.getItems(); + assert.strictEqual(items.length, 2, "two items in the Menu"); + assert.strictEqual((items[0] as MenuItem).getText(), "One", "first item text"); + assert.strictEqual((items[1] as MenuItem).getText(), "Two", "second item text"); + view.destroy(); + } + } +); + +QUnit.test( + "Menu with children outside a view — still works (smoke test)", + (assert) => { + const menu = jsx(Menu, { + children: [ + jsx(MenuItem, { text: "A" }), + jsx(MenuItem, { text: "B" }), + ] + }) as Menu; + + const items = menu.getItems(); + assert.strictEqual(items.length, 2, "two items in Menu"); + assert.strictEqual((items[0] as MenuItem).getText(), "A", "first item text"); + assert.strictEqual((items[1] as MenuItem).getText(), "B", "second item text"); + menu.destroy(); + } +);