Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugin.needs to assert dependencies #89

Merged
merged 4 commits into from
Jan 6, 2024
Merged

plugin.needs to assert dependencies #89

merged 4 commits into from
Jan 6, 2024

Conversation

staltz
Copy link
Member

@staltz staltz commented Jan 5, 2024

Context

In SSB and PPPPP it's common for SS plugins to need other plugins, so we have checks all over the place like this:

Problem

There's a lot of code repetition across various libraries, and it's easy to forget to check for one of these dependencies. Also, the custom error messages can differ from each other, and this can make it harder to detect that they are in fact the same kind of error.

Solution

Fixing this in secret-stack is not hard, as you can see this is a small PR that adds just ~6 lines of code. Plugins would declare what they need, like this:

module.exports = {
  name: 'bluetooth',
  needs: ['db', 'conn'], // <-----------
  manifest: {
    localPeers: 'async',
    updates: 'source'
  },
  init: (ssb, opts) => {
    // .. do things
  }
}

If a plugin called db was not loaded, then an error will be thrown:

Error: secret-stack plugin "bluetooth" needs plugin "db" but not found

Considerations

needs looks like depject!

Hold on, it's different this time. Unlike depject, we are not injecting dependencies, and we don't have that whole web of "gives". All that's happening here is a small DSL for generating checks of presence of other plugins. Effectively, those checks already exist today! i.e. if you load such a plugin bluetooth and attempt to use ssb.db.add, it will error with no property 'db' on undefined. All that this PR is doing is making those errors more friendly and more debuggable.

This new plugin.needs metadata could also be useful in the future. We could implement secret-stack such that the order of loading plugins doesn't matter because we can traverse this DAG of "needs" and load them in topological order.

@staltz staltz requested a review from mixmix January 5, 2024 13:10
@Powersource
Copy link

Ooh this sounds nice. There are cases where stuff isn't assumed to be there immediately, or at all, like https://github.com/ssbc/ssb-box2/blob/7eba58bcb265250c1928bd7964ac2f6e31a74155/index.js#L18-L25 but this PR is probably a nice improvement even if it doesn't handle those cases.

Copy link

@Powersource Powersource left a comment

Choose a reason for hiding this comment

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

Just needs a readme mention otherwise cool!

@staltz
Copy link
Member Author

staltz commented Jan 5, 2024

Ooh this sounds nice. There are cases where stuff isn't assumed to be there immediately, or at all, like https://github.com/ssbc/ssb-box2/blob/7eba58bcb265250c1928bd7964ac2f6e31a74155/index.js#L18-L25 but this PR is probably a nice improvement even if it doesn't handle those cases.

This is actually a great example to bring attention to, because I realize that a tweak to this PR could handle those cases too. Essentially, we just need to do a queueMicrotask(() => (similar to setTimeout) so that these needs assertions run after all plugins are loaded but before any of the plugins init runs. In fact why not, I'll just modify this PR (and add an extra test).

Just needs a readme mention otherwise cool!

Good point. I forget this. Every single time.

@staltz
Copy link
Member Author

staltz commented Jan 5, 2024

Done, both

Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

Haven't seen queueMicrotask before. Nice!
Love the "wait it's not full depject!"

@@ -87,6 +88,15 @@ will be available at `node.fooBar`.
A `plugin.name` can also be an `'object`. This object will be merged
directly with the

### `plugin.needs` (Array) _optional_

An array of strings which are the names of other plugins that this plugin
Copy link
Member

Choose a reason for hiding this comment

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

Name the plugin is loaded under? (conn, not ssb-conn)

Copy link
Member Author

Choose a reason for hiding this comment

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

So far whenever we talk about plugin names in this document, it's always plugin.name and not the package name.

@staltz staltz merged commit 6f1c104 into main Jan 6, 2024
5 checks passed
@staltz staltz deleted the plugin-needs branch January 8, 2024 08:59
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.

3 participants