Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .changeset/fix-forwarded-aggregation.md
Original file line number Diff line number Diff line change
@@ -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.
19 changes: 19 additions & 0 deletions .changeset/log-view-errors.md
Original file line number Diff line number Diff line change
@@ -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.
9 changes: 9 additions & 0 deletions packages/jsx-runtime-showcase/samples.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 <For>, <If>", 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 <Switch>", icon: "sap-icon://switch-classes", route: "exploreSample", arg: "switch-plugin" }
Expand Down
Original file line number Diff line number Diff line change
@@ -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
`<Menu-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 = (
<Menu title="Actions">
<MenuItem text="Refresh" icon="sap-icon://refresh" />
<MenuItem text="Download" icon="sap-icon://download" />
</Menu>
) as Menu;
// ...later
menu.openBy(button);

// Pattern 2 — List contextMenu (aggregation prop, items also forwarded)
<List
contextMenu={
<Menu>
<MenuItem text="Edit" icon="sap-icon://edit" />
<MenuItem text="Delete" icon="sap-icon://delete" />
</Menu>
}
items={{ path: "/fruits" }}
>
<StandardListItem title="{name}" />
</List>
```

Neither `Menu` carries an explicit `id`.

See also: [aggregations](#/learn/aggregations) — how the runtime
maps JSX children to UI5 aggregations.
17 changes: 17 additions & 0 deletions packages/jsx-runtime-showcase/webapp/view/ExploreSample.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -154,6 +155,12 @@ const REGISTRY: Record<string, {
icon: "sap-icon://key-user-settings",
viewName: "ui5.community.jsx.showcase.view.showcases.CustomDataKey",
sourcePath: "view/showcases/CustomDataKey.tsx"
},
"forwarded-menu": {
title: "Forwarded aggregation (Menu)",
icon: "sap-icon://menu2",
viewName: "ui5.community.jsx.showcase.view.showcases.ForwardedMenu",
sourcePath: "view/showcases/ForwardedMenu.tsx"
}
};

Expand Down Expand Up @@ -324,6 +331,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(
`<div class="jsx-showcase-explore-description">` +
`<p><i>Could not load docs/samples/${escapeHtml(sampleId)}.md: ${escapeHtml(String(error))}</i></p>` +
Expand All @@ -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)}`
Expand Down
112 changes: 112 additions & 0 deletions packages/jsx-runtime-showcase/webapp/view/showcases/ForwardedMenu.tsx
Original file line number Diff line number Diff line change
@@ -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 `<MenuItem>` 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 `<Menu>` built from `<MenuItem>` JSX children (the default-
* aggregation / forwarded path that was previously broken).
* 2. **List context menu** — a `<List>` with `contextMenu={<Menu>…</Menu>}`
* (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 = (
<Menu title="Actions">
<MenuItem text="Refresh" icon="sap-icon://refresh" />
<MenuItem text="Download" icon="sap-icon://download" />
<MenuItem text="Share" icon="sap-icon://share" />
</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 = (
<Menu>
<MenuItem text="Edit" icon="sap-icon://edit" />
<MenuItem text="Delete" icon="sap-icon://delete" />
</Menu>
) as Menu;

return (
<VBox class="sapUiSmallMargin">
{/* ── Section 1: standalone button menu ──────────────── */}
<Title text="Standalone Menu (button)" level="H4" class="sapUiSmallMarginBottom" />
<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());
}
}
Loading
Loading