rusackas opened a new issue, #41779: URL: https://github.com/apache/superset/issues/41779
*Please make sure you are familiar with the SIP process documented* [here](https://github.com/apache/superset/issues/5602). The SIP will be numbered by a committer upon acceptance. ## [SIP] Proposal for registering built-in implementations as default-tier providers in extension contribution points ### Motivation [SIP-151](https://github.com/apache/superset/issues/31932) gave Superset an extensions architecture with contribution points that let extensions replace built-in functionality — the SQL Lab editor can be swapped for a Monaco-based one, and with [#41703](https://github.com/apache/superset/pull/41703) the entire dashboard renderer can be swapped as well. Extensions are gated behind the `ENABLE_EXTENSIONS` feature flag, which is **off by default** — and core surfaces like dashboard rendering and SQL editing must obviously keep working when it is off. Today that guarantee is provided by **hardcoded fallbacks inside host components**. `EditorHost` checks the editor registry and, when no extension has registered a provider for the language, falls back to a hardcoded `AceEditorProvider`. This works, but it has structural drawbacks: 1. **The built-in bypasses the contribution point it anchors.** The extension path and the built-in path are two different code paths in the host component, so the contract is never exercised by Superset itself — extensions are the only consumers, and contract regressions only surface downstream. 2. **Introspection is misleading.** `editors.getEditor('sql')` returns `undefined` even though an editor is very much active — the API can't tell you what is actually rendering. 3. **Extensions can only replace, never augment.** There is no way for an extension to retrieve the built-in implementation and wrap it (add chrome around the default dashboard renderer, decorate the default editor) because the built-in isn't reachable through the API. 4. **Fallback logic is duplicated per host** and each host invents its own variant, rather than the registry owning resolution. We want built-ins to be first-class citizens of the extension system — "Superset ships with a default dashboard-renderer extension" rather than "the host has an if-statement" — without ever making core UX dependent on the extensions feature flag or the extension loading machinery. ### Proposed Change Adopt a **two-tier, single-slot provider registry** pattern for contribution points that replace built-in functionality, as implemented for the dashboard renderer in [#41703](https://github.com/apache/superset/pull/41703): - **Default tier (host):** the built-in implementation is registered *through the contribution point itself*, by the host, as the default provider (e.g. `superset.dashboard-renderer`). Registration happens as a lazy side-effect module import wherever the surface renders (app and embedded entry points alike), fully independent of `ENABLE_EXTENSIONS`, `ExtensionsStartup`, and the module-federation loader. The component itself is loaded via `React.lazy`, so registering the default does not pull the implementation into the startup bundle. - **Override tier (extensions):** extensions register through the existing public API (`registerDashboardRenderer`, `registerEditor`, ...). At most one override occupies the slot; the most recent registration wins and displaces the previous *override* — never the default. - **Resolution lives in the registry:** `getProvider()` returns `override ?? default`. Disposing the active override falls back to the default through the registry, not through a host code branch. The host component simply renders the resolved provider (extension overrides wrapped in an error boundary; edit-mode and feature-flag rules applied to overrides only). - **The feature flag gates overrides, not defaults.** With `ENABLE_EXTENSIONS` off, the loader never loads extensions and the host ignores overrides — the default renders unconditionally. A misbehaving or absent extension system can never take dashboards down. - **Augmentation becomes possible:** the public API exposes the default provider (e.g. `dashboards.getDefaultDashboardRenderer()`), so an extension can wrap the built-in component in its own and register the wrapper — replace *or* augment. Registry sketch (from the dashboard-renderer implementation): ```ts class DashboardRendererProviders { private provider?: DashboardRendererProvider; // override tier private defaultProvider?: DashboardRendererProvider; // default tier setDefaultProvider(renderer, component): void; // host-internal, idempotent by id registerProvider(renderer, component): Disposable; // public extension API getDefaultProvider(): DashboardRendererProvider | undefined; getOverrideProvider(): DashboardRendererProvider | undefined; getProvider(): DashboardRendererProvider | undefined; // override ?? default } ``` The dashboard renderer ships this pattern already (merged into the PR branch of #41703, with unit, integration, and Playwright E2E coverage — including a live assertion that `getDashboardRenderer()` reports `superset.dashboard-renderer` before any extension registers). This SIP proposes we: 1. Ratify the pattern as **the standard for contribution points that replace built-in functionality**, documented in the extensions developer docs. 2. Migrate the existing single-slot/keyed points to it: `editors` (register `AceEditorProvider` as the default provider per language, remove the hardcoded fallback from `EditorHost`) and `chat` (no built-in exists today; the default tier simply stays empty, which the pattern supports). 3. Apply it to future contribution points that wrap existing surfaces (e.g. chart renderers, filter bars) as they are proposed. ### New or Changed Public Interfaces - `dashboards.getDefaultDashboardRenderer(): DashboardRendererProvider | undefined` (shipped with #41703). - `dashboards.getDashboardRenderer()` semantics: returns the *active* provider — the extension override when one is active, otherwise the built-in default (previously `undefined` when no extension registered). - Proposed follow-ups in the same shape: `editors.getDefaultEditor(language)`, with `editors.getEditor(language)` returning the active provider including the default. - Reserved, well-known provider ids for built-ins under the `superset.` prefix (e.g. `superset.dashboard-renderer`, `superset.ace-editor`). - No REST endpoints, models, CLI, or deployment changes. The `setDefaultProvider` registration API is host-internal and deliberately **not** exposed on `window.superset`. ### New dependencies None. ### Migration Plan and Compatibility No database migrations. The pattern is behavior-compatible: with no extensions registered, the default provider renders exactly what the hardcoded fallback rendered before. Migrating `editors` is an internal refactor of `EditorHost` + `EditorProviders`; the only observable change is `getEditor()` becoming truthful about the active editor, which is a strict improvement but should be noted in the extensions changelog since extensions may have used `getEditor() === undefined` as a "no custom editor" check (they should use the provider id or a new `getOverride`-style accessor instead). ### Rejected Alternatives 1. **Hardcoded fallback in the host component (the current SQL editor approach).** `EditorHost` falls back to `AceEditorProvider` in a code branch, and the dashboard renderer initially shipped the same shape. Rejected as the long-term pattern for the reasons in Motivation: the contract isn't dogfooded, introspection APIs return `undefined` while a built-in renders, augmentation is impossible, and every host duplicates its own fallback logic. It does have the virtue of simplicity, and nothing breaks if a given contribution point stays on it temporarily — this SIP treats it as the migration starting point, not an error. 2. **Shipping built-ins as real packaged extensions** (bundled `.supx` files loaded through `ExtensionsLoader` / module federation). Maximally uniform, but it would route core UX through the extension loading machinery — remote-entry fetches, manifest parsing, and the `ENABLE_EXTENSIONS` gate — for code that is already statically bundled. A failure anywhere in that machinery (or simply the flag being off, which is the default) would take down dashboards and SQL Lab. Rejected: core surfaces must render even if the extension system is disabled or broken. 3. **Enabling `ENABLE_EXTENSIONS` by default** so registry-based built-ins always load. Rejected: it conflates two decisions (whether operators opt into third-party extensions vs. whether built-ins render), expands the default security surface, and still leaves built-ins dependent on loader machinery. -- 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]
