michael-s-molina commented on issue #45248:
URL: https://github.com/apache/superset/issues/45248#issuecomment-6343209367
Thanks for consolidating this with #44101. The direction makes sense to me:
reusing the `menus` and `views` registries instead of adding a parallel one. I
have feedback in three areas.
## 1. Motivation overstates the current gap
> "An extension that owns a surface of its own (an admin console, a catalog
browser, a governance report, SIP-96's notification manager) has nowhere to
live and no way to be reached."
Extensions already own surfaces today. SQL Lab extensions contribute views
and panels that users reach. What is missing is a standalone page: a full-page
surface with its own URL and a navigation entry. Suggested wording:
> An extension that needs a full page of its own (an admin console, a
catalog browser, a governance report, SIP-96's notification manager) has no
host surface to mount on and no route to be reached at. Existing contribution
points attach to a host feature such as SQL Lab; none provides a standalone,
deep-linkable page.
The next paragraph makes a similar claim:
> "Nothing in the app shell outside SQL Lab renders extension contributions."
Other surfaces also render contributions today (editors, chat, semantic
layers). What the app shell itself doesn't do is read from `menus` for the main
navbar or settings menu, and it has no route an extension-registered view can
be mounted on. Suggested wording:
> Several host surfaces already render extension contributions (SQL Lab,
editors, chat, semantic layers), but the app shell itself doesn't: neither the
main navbar nor the settings menu reads from the `menus` registry, and there is
no route an extension-registered view can be mounted on.
The same paragraph also says "SQL Lab is the only host surface that reads
from them", which needs the same correction. Broadening this to the other
consumers would strengthen the case for reusing the registries.
## 2. Do we need `page` as a separate concept?
I don't think so. A view is already "any React component registered at a
location": `registerView(view, location, component)` takes an arbitrary
`ComponentType`. Not every view is a page, but a page is a view. Registering a
view at `global.page` is what makes it a page. Beyond that registration, the
only thing missing is a backend endpoint that loads the SPA for those pages.
After that, the view is resolved and rendered like any other view in the app,
with the same location keying, reactivity and `ErrorBoundary` wrapping, and no
new type.
As written, `MenuItem.page` is only a navigation target plus a label taken
from the view's `name`. That is a command ("open this view") with a lookup, not
a new kind of thing.
Proposal:
- Keep `global.page` as the explicit location where pages are registered, so
only views registered there are URL-addressable.
- Add a built-in host command, `core.navigation.openView`, that navigates to
a view registered at `global.page` by its id. Its handler calls `navigateTo`.
- Contribute the navbar entry with `command: 'core.navigation.openView'`
plus `icon` and `order`, and drop `MenuItem.page`.
This keeps `MenuItem.view` meaning "hosting view" and removes the "exactly
one of `page`/`command`/`href`" rule. It also makes the "target not registered"
case the same as "command not registered", and it lets keybindings and other
menu locations open the page for free. The cost is that `MenuItem` needs a way
to pass the view id to the command, since it carries no arguments today.
On `href`: with this model it is only needed for external links, and a
command handler can already open one. I'd defer it from the first cut (open
question 4), which also avoids having to specify scheme restrictions for
extension-supplied URLs.
## 3. The backend route
I read the current routes. The only extension-related FAB view is
`ExtensionsView` (`superset/extensions/view.py`), serving `/extensions/list/`
behind `@has_access` with the `Extensions` class permission. The REST side is
`/api/v1/extensions/...` for the host and `/extensions/{publisher}/{name}/...`
for extension `@api` resources.
- **Name.** Views are a generic concept: SQL Lab panels are views too.
`/extensions/view/<id>` suggests any view id works. I'd use
`/extensions/pages/<id>`, as another method on `ExtensionsView` next to `list`,
matching the `global.page` location. The client page host should resolve only
views registered at `global.page`.
- **Gating.** The SIP compares the shell route to `welcome` (login only),
but the sibling `/extensions/list/` uses `@has_access` and the `Extensions`
permission. Why not reuse that permission? That also bears on the "no per-item
RBAC" limitation. Which roles have `can_read on Extensions` by default?
- **Reserved names.** The host-owned `/extensions/list/` and
`/extensions/pages/<id>` share a prefix with the extension-owned
`/extensions/<publisher>/<name>/...`. Publisher names `list` and `pages` need
to be reserved and enforced at install time.
- **Open question 5.** The server returns only the shell, so it can't tell
which ids exist. I'd always render the shell and let the client show not-found.
- **Path parameter.** `:viewId+` allows slashes. If ids are dotted
(`acme.catalog.main`), a plain segment avoids needing `<path:...>` and keeps
trailing-slash handling simple.
## Smaller points
- `MenuItem` is only documented as "exactly one of …". A discriminated union
plus runtime validation in `registerMenuItem` would fail loudly on author
mistakes instead of silently skipping.
- Adding `'extension'` to the `Page` union can break exhaustive `switch`
statements in existing extensions, so I'd mention it in the migration notes.
- Is view-id uniqueness enforced across extensions? The registry is a `Map`,
so a duplicate id silently overwrites.
- The question numbering is swapped (6 appears before 5).
## Possible follow-up
A backend-side declaration for pages, similar to how `@tool` carries
`class_permission_name`, could provide per-page permissions and bootstrap-time
nav items (fixing the late paint). It would be optional and keyed on the same
view id, so frontend-only extensions keep working. It would be good for the SIP
to say whether this design leaves room for 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]
