diff --git a/.changeset/fix-forwarded-aggregation.md b/.changeset/fix-forwarded-aggregation.md new file mode 100644 index 0000000..d7039d9 --- /dev/null +++ b/.changeset/fix-forwarded-aggregation.md @@ -0,0 +1,16 @@ +--- +"@ui5-community/jsx-runtime": patch +--- + +Fix: auto-prefix-id no longer prefixes UI5-generated ids (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/.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/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 acf2d79..356bc54 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"; @@ -154,6 +155,12 @@ const REGISTRY: Record { 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 +362,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-showcase/webapp/view/showcases/ForwardedMenu.tsx b/packages/jsx-runtime-showcase/webapp/view/showcases/ForwardedMenu.tsx new file mode 100644 index 0000000..74ad859 --- /dev/null +++ b/packages/jsx-runtime-showcase/webapp/view/showcases/ForwardedMenu.tsx @@ -0,0 +1,112 @@ +import View from "sap/ui/core/mvc/View"; +import type Control from "sap/ui/core/Control"; +import VBox from "sap/m/VBox"; +import HBox from "sap/m/HBox"; +import Title from "sap/m/Title"; +import Text from "sap/m/Text"; +import Button from "sap/m/Button"; +import List from "sap/m/List"; +import StandardListItem from "sap/m/StandardListItem"; +import Menu from "sap/m/Menu"; +import MenuItem from "sap/m/MenuItem"; +import JSONModel from "sap/ui/model/json/JSONModel"; + +/** + * # Forwarded aggregation: `sap.m.Menu` via JSX (issue fix demo) + * + * `sap.m.Menu` declares its default `items` aggregation as + * **forwarded** — the real backing store is an internal `MenuWrapper` + * control created in `init()` and resolved by id at add-time. Without + * the runtime fix, passing `` 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/installViewScopeBridge.ts b/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts index 5056f62..c4b81c7 100644 --- a/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts +++ b/packages/jsx-runtime/src/runtime/installViewScopeBridge.ts @@ -1,5 +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"; /** @@ -15,36 +17,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 +66,85 @@ let installed = false; export function installViewScopeBridge(): void { if (installed) return; installed = true; - const original = View.prototype.createContent as ( - this: View - ) => Control | Control[] | Promise<Control | Control[]>; - View.prototype.createContent = function patchedCreateContent( - this: View - ): Control | Control[] | Promise<Control | Control[]> { - // 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<Control | Control[]>; + 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<Control | Control[]> { + return withScope({ view: this }, () => { + let result: Control | Control[] | Promise<Control | Control[]>; + try { + result = subclassCreateContent.call(this); + } catch (error) { + return logAndRethrow(error); + } + if (result instanceof Promise) { + return result.catch(logAndRethrow); + } + return result; + }); + }; + + // 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 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/runtime.ts b/packages/jsx-runtime/src/runtime/runtime.ts index dd29eec..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"; @@ -402,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 new file mode 100644 index 0000000..5cdf9f8 --- /dev/null +++ b/packages/jsx-runtime/test/qunit/runtime/forwarded-aggregation.qunit.ts @@ -0,0 +1,110 @@ +/** + * QUnit regression tests for issue #9: the auto-prefix-id feature must not + * prefix UI5-generated ids. + */ +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"); + +/** + * 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(); + } +); 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<void>): Promise<void> { + 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: <Switch> / <Case> / <Default>" } } };