amaannawab923 commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4391135669
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -199,6 +200,108 @@ const StyledImage = styled(Image)`
const { useBreakpoint } = Grid;
+/**
+ * What the two helpers below need of a contributed page, and no more, so the
+ * menu does not take a dependency on the pages registry. One shape shared by
+ * both, so a field one of them reads cannot go missing from the other.
+ */
+type ContributedPage = {
+ name: string;
+ path: string;
+ nav?: { label?: string; category?: string };
+ permitted?: boolean;
+};
+
+/**
+ * Folds pages contributed by extensions into the server-rendered menu.
+ *
+ * The navbar is built on the server from FAB's menu, which an extension
+ * cannot write to: an entry there needs a `menu_access` permission created
+ * at boot, and extensions register in the browser. A page that opted in with
+ * `nav` is therefore added here, client-side, so that a contributed page has
+ * somewhere a user could find it at all.
+ *
+ * A page without `nav.category` sits at the top level. One that names a
+ * category goes into that group of the Settings menu instead, which is
+ * where an administrative screen belongs -- see `withExtensionSettings`.
+ */
+export function withExtensionPages(
+ menu: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ // A page the user may not open is left out rather than shown and refused:
+ // the server answers 403 for it, so an entry here would only advertise a
+ // dead end. The page stays registered, so a direct link still explains
+ // itself rather than rendering nothing.
+ const contributed = pages.filter(
+ page => page.nav && !page.nav.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return menu;
+ }
+
+ return [
+ ...menu,
+ ...contributed.map(
+ page =>
+ ({
+ label: page.nav?.label ?? page.name,
+ // `name` is the React key for the entry, and the label is the
+ // extension author's to choose -- a page labelled "Charts" would
+ // collide with the host's own entry. The path cannot collide,
+ // because the server refused any page that shared one.
+ name: `extension:${page.path}`,
Review Comment:
Fixed in 37caec2ce4.
`activeTabs` now asks the page registry, before the host cases rather than
in the `default` branch. Asking the registry keeps one definition of which
paths a page owns — `isFrontendRoute` and `derivePage` read the same one — and
it matches by longest path on segment boundaries, so `/reports/archive` wins
over `/reports` and `/reports-weekly` belongs to neither.
Ordering it ahead of the host cases also closes something adjacent that your
comment did not cover: three of those cases match by unbounded prefix, so a
page declared at `/dashboard-review` was underlining **Dashboards**. That path
is legal and the server would register it. Putting the registry first cannot
cost a host screen its own highlight, because the server refuses a page whose
path it already serves and a refused page is never registered. `/dataset` had
already been hardened against this class; the others had not, and I have left
tightening them to a separate change rather than altering host highlighting
from this PR.
`contributedPages` joins the effect's dependency array, or a cold load onto
a contributed page would never highlight its entry — there is no navigation to
re-run the effect once the bundle registers.
One thing worth recording: a Settings-menu entry is deliberately not fixed,
because it cannot be. `RightMenu` renders both its menus `selectable={false}`
and passes no `selectedKeys`, so no Settings child is ever highlighted,
contributed or host. Contributed Settings entries are therefore already at
parity, and this change is navbar-only.
Separately, and not addressed here: `withExtensionPages` is applied only at
the desktop nav call site, so the mobile drawer receives the unmodified menu
and shows no contributed entries at all. That matches a bot comment on the same
file.
Six tests; five fail against the previous code.
##########
superset-extensions-cli/src/superset_extensions_cli/templates/frontend/webpack.config.js.j2:
##########
@@ -61,6 +61,15 @@ module.exports = (env, argv) => {
singleton: true,
import: false,
},
+ // Declared here as well as by the host, because Module Federation
+ // only consumes the share scope for modules the remote itself
+ // declares as shared. Without this the extension bundles its own
+ // copy, whose SupersetClient is never configured -- so every call
+ // through it goes out without a CSRF token.
+ "@superset-ui/core": {
+ singleton: true,
+ import: false,
+ },
Review Comment:
Partly fixed in 37caec2ce4, and you are right on both halves — I checked
them against npm directly.
**The documentation half is fixed.** Three places told authors to write that
import as though it would compile: the published `.d.ts` in
`packages/superset-core/src/pages/index.ts`, the `contribution-types.md`
section on calling Superset's API, and the `UPDATING.md` note. A `shared` entry
is a runtime arrangement TypeScript never sees, so the import cannot resolve
until the package is installed locally as well. The project's own rule already
says this — `quick-start.md` requires a shared package in both
`peerDependencies` and `devDependencies` — it simply was not applied to this
one. `architecture.md` and `quick-start.md` were also still describing the
share set without `@superset-ui/core`; both now list it, and `architecture.md`
says plainly that only `@apache-superset/core` is rebound per extension.
I also documented the trap behind your version point, which is sharper than
a stale release. The copy on npm and the copy this host runs **carry the same
version number**, `0.20.4`, with about two years between them. The published
root index exports `./translation`; this repo's no longer does. So `import {
SupersetClient, t } from '@superset-ui/core'` type-checks against the published
types and `t` is `undefined` at runtime — and since neither side of the share
declares a `requiredVersion`, nothing warns. Not npm, not webpack, not `tsc`.
**Two halves are not fixed, deliberately:**
- The scaffold still does not declare the dependency. Whether it should
depends on whether the share survives at all — your comment on
`webpack.config.js` argues it should not, and if that is where we land the
scaffold should stop mentioning the package rather than start declaring it.
Those are mutually exclusive, so I would rather settle that one first than do
both in turn.
- `@apache-superset/core` on npm is `0.1.0` and has no `pages` export — nor
`chat`, `navigation` or `storage`. The repo's package is also `0.1.0`, and npm
does not overwrite, so no code change in this PR can close it: it needs a
version bump plus the manual publish step in `RELEASING/README.md`. Agreed it
is real and agreed it is out of scope here; worth its own release task, since
four namespaces are waiting on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
