Skip to content

ui: Add defaultPlugins to Embedder interface#5350

Open
dreveman wants to merge 1 commit intomainfrom
dev/reveman/embedder-plugin-filter
Open

ui: Add defaultPlugins to Embedder interface#5350
dreveman wants to merge 1 commit intomainfrom
dev/reveman/embedder-plugin-filter

Conversation

@dreveman
Copy link
Copy Markdown
Collaborator

@dreveman dreveman commented Mar 30, 2026

Allow embedders to control which plugins are enabled by default by
adding a required defaultPlugins property to the Embedder interface.
Both existing embedders return the built-in default list, preserving
current behavior while letting third-party embedders customize it.

Move default_plugins.ts into the embedder folder and pass the
default plugin list to PluginManagerImpl via the constructor.

@dreveman dreveman requested a review from a team as a code owner March 30, 2026 18:26
@dreveman
Copy link
Copy Markdown
Collaborator Author

It would be nice to keep this in the embedder implementation instead of having to modify the default list. But maybe there's a way to do this already that I missed?

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 30, 2026

@LalitMaganti
Copy link
Copy Markdown
Member

Wouldn't it be far better to just move the default plugin list to the embedder interface? That seems like a no-brainer instead of adding a lambda for this.

@dreveman
Copy link
Copy Markdown
Collaborator Author

Wouldn't it be far better to just move the default plugin list to the embedder interface? That seems like a no-brainer instead of adding a lambda for this.

I would like to avoid having the embedder be aware of all default plugins. Eg. the change we made to move some gpu related code from TPTrack plugin to a new Gpu plugin shouldn't require all downstream embedders to update their code. But maybe you're suggesting moving into the embedder in a way that would allow this?

@LalitMaganti
Copy link
Copy Markdown
Member

I mean ultimately in my mind how the embedder chooses to modify the embedded list is an implementation detail. You are free to depend on the upstream embedded definition and modify it or wholesale replace it.

I just don't think the API should be a lambda, that just feel weird to me.

@dreveman dreveman force-pushed the dev/reveman/embedder-plugin-filter branch from 29d2a06 to 04ffb68 Compare March 30, 2026 19:24
@dreveman
Copy link
Copy Markdown
Collaborator Author

I mean ultimately in my mind how the embedder chooses to modify the embedded list is an implementation detail. You are free to depend on the upstream embedded definition and modify it or wholesale replace it.

I just don't think the API should be a lambda, that just feel weird to me.

Ah, done. Ptal.

@dreveman dreveman force-pushed the dev/reveman/embedder-plugin-filter branch from 04ffb68 to 4e3c53c Compare March 30, 2026 20:40
@dreveman dreveman requested a review from LalitMaganti March 30, 2026 20:40
@dreveman dreveman changed the title ui: Add filterDefaultPlugins to Embedder interface ui: Add defaultPlugins to Embedder interface Mar 30, 2026
Copy link
Copy Markdown
Member

@LalitMaganti LalitMaganti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but will leave to Steve to confirm he's happy.

Allow embedders to control which plugins are enabled by default by
adding a required defaultPlugins property to the Embedder interface.
Both existing embedders return the built-in default list, preserving
current behavior while letting third-party embedders customize it.

Move default_plugins.ts into the embedder folder and pass the
default plugin list to PluginManagerImpl via the constructor.
@dreveman dreveman force-pushed the dev/reveman/embedder-plugin-filter branch from 4e3c53c to d9c9e80 Compare March 30, 2026 21:30
@dreveman
Copy link
Copy Markdown
Collaborator Author

Looks good to me but will leave to Steve to confirm he's happy.

@stevegolton Let me know how this looks to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants