Repository navigation
Plugin manager review - #879
Open
johngeorgewright wants to merge 10 commits into
Open
johngeorgewright wants to merge 10 commits into
johngeorgewright wants to merge 10 commits into
Conversation
`#pluginsToEnable` was a single instance field overwritten on every call to `enablePlugins`. Because `enablePlugins` is re-entrant (handed to plugins via the enable context) and runs its children concurrently, nested and concurrent calls clobbered the field mid-iteration. In particular, enabling a plugin's hard dependencies replaced the set that optional-dependency resolution reads, so an optional dependency requested in the same batch was silently not recognised — and the depender's `enable()` no longer waited for it. Thread the requested set through `#enablePlugin`/`#enableOptionalDependencies` as batch-local state, and track in-flight batches in `#pendingBatches` so `disablePlugins` can still cancel a plugin that is disabled mid-enable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`#addPlugin` resolved each dependency name to its Plugin object eagerly via `#getPlugin`, which threw when the dependency hadn't been registered yet. That forced consumers to register plugins in strict dependency order. Wire a dependency edge immediately when the dependency is already registered, otherwise record it and wire it up when the dependency is later registered. A dependency that is never registered is handled as before at enable time (a warning, and it's skipped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gins `enablePlugins` used to `console.warn` and continue on an unregistered name while `disablePlugins` threw — inconsistent, and the console logging wasn't something consumers could control. Both paths now route through a single `#handleUnknownPlugin` that throws by default. A new `onUnknownPlugin(pluginName, phase)` option lets consumers log, collect, or otherwise tolerate unknown plugins; if the handler doesn't throw, the name is skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously a plugin that threw from enable()/run() rejected the whole batch via Promise.all, and a failed enable left the plugin marked as enabled (it was added to #enabledPlugins optimistically, before enable ran). Catch errors per plugin so siblings keep going, roll a failed enable back out of #enabledPlugins (and abort it), and surface the error through a new onPluginError(error, plugin, phase) option. It defaults to console.error; throwing from the handler restores fail-fast behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`#runPlugin` resolves a plugin's dependencies before marking it as run, so a dependency cycle recursed forever (the enable phase is already guarded by the `#enabledPlugins` check). Track the dependency path down the recursion and throw a clear "Circular dependency detected: a -> b -> a" error instead of hanging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The enable phase orders optional dependencies, but run() only waited on hard dependencies, so a present optional dependency could run after its depender. Fold enabled optional dependencies into the run ordering (skipping ones that aren't enabled, so they still don't gate the run), sharing the same circular -dependency detection as hard dependencies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
withOptions copied the #abortControllers WeakMap into the clone but not #enabledPlugins/#ran. Because the abort listeners captured the manager that created each controller, aborting or disabling a plugin through the clone mutated the original manager's state. Share only the static registration (#plugins, #dependencyGraph) and let the clone keep its own runtime state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Remove the unused `DependencyGraph.names()` generator (never called, and `DependencyGraph` isn't exported); it was the only in-package caller of the misspelled `Graph.verticies()`. - Default `Plugin`'s context generics to `Record<string, unknown>` instead of `any`, so a bare `Plugin` keeps context type-safety. Existing consumers pass the type arguments explicitly and are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BREAKING CHANGE: `amendKey` and `filter` have been removed in favour of `set`.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.