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

Questions: Should we recommend self-registering plugins or manually registering plugins? #122

Closed
aaronfrost opened this issue Dec 31, 2019 · 8 comments
Labels
enhancement New feature or request

Comments

@aaronfrost
Copy link
Contributor

aaronfrost commented Dec 31, 2019

Self Registered Plugins vs Manually Registered Plugins

When people build a plugin for Scully, should we recommend a self registering plugin or a plugin that they will still need to call registerPlugin on?


// in your scully.config.js

// Self Registered Plugins 
require("community-plugin");

// Manually Register Plugins
const communityPlugin = require("community-plugin");
registerPlugin('router', 'somenameofmychoosing', communityPlugin);

Which of these options would be the best? I am leaning towards Self Registered Plugins.

@aaronfrost aaronfrost added the enhancement New feature or request label Dec 31, 2019
@aaronfrost
Copy link
Contributor Author

To add clarity: you would consume both of these the same way.

For a plugin that queries a CMS or an API somewhere, your sample plugin config looks the same for both of these examples:

// In your scully.config.js
exports.config = {
    ...,
    routes: {
        "/sample/:sampleId": {
            type: "samplePlugin",   // or the name you gave it when you manually register it
            sampleId: {
                cmsCredentials: {
                    publicKey: "1234",
                    privateKey: "abcd",
                },
                query(db){
                    return db.collection('sampleIds'); // in the database was firebase... example
                },
                property: 'item.id'
            }
        }
    },
}

@aaronfrost
Copy link
Contributor Author

@SanderElias @beeman @LayZeeDK @SanderElias @jorgeucano @Splaktar what do you all think?

@LayZeeDK
Copy link
Contributor

Self-registering plugins would make them easy to consume.

Manually registered plugins might allow for some flexibility such as the type name in your example or configuration options, for example to preregister some common option like API path and credentials.

@SanderElias
Copy link
Contributor

The problem with "self-registration" is that somehow Scully needs to be able to pull those in.
That requires a list of available plugins somewhere. Such a list also needs maintenance.
First of all, plugins themselves need to put their main in this list.
Also, if a plugin is removed it needs to be taken off this list.
The list itself needs to be in a common-place, like angular.json, package.json or scully.config.js

I would like to propose a middle ground, that doesn't need a list, and makes it easy to pull them in:
in scully config.js:

require('./extraPlugin/extra-plugin.js')(registerPlugin);

in ./extraPlugin/extra-plugin.js'

/** plugin code here **/
module.exports = registerPlugin => registerPlugin('router', 'extra', extraRoutesPlugin);

Works with #107
This would keep the flexibility and would put in the "list" inside the scully.config.js
Also, if we go this route, I can provide something like this:

npm run scully addPluginToConfig "myNpmProvidedPlugin"

which will add the line:

require("myNpmProvidedPlugin")(registerPlugin);

to the scully.config.js if is not yet there.
Plugin authors can add this to their package.json install script.

@aaronfrost
Copy link
Contributor Author

After talking to you this evening, I feel like simplifying the codebase by removing the dependency on vm will make this problem largely go away. If all contexts point to the same version of registerPlugin, then we will eliminate our errors here.

Sander and I spoke about the advantages of using vm and running the community/plugin code inside of the vm. It protects us from cowboy code that can break us. But it doesn't solve the bigger, harder to fix issues that will eventually arrive. So for now, it seems like we may punt and remove vm and use a simple require for now.

SanderElias added a commit that referenced this issue Dec 31, 2019
This removes the protective VM context, to make creating plugins a bit simpler

closes #122 re #107
@LayZeeDK
Copy link
Contributor

LayZeeDK commented Jan 1, 2020

Are the docs still representative of how you want plugins to be registered? https://github.com/scullyio/scully/blob/master/docs/plugins.md

@SanderElias
Copy link
Contributor

SanderElias commented Jan 1, 2020

@LayZeeDK, yes no change there. However you need to import/require registerPlugin now.
(have a look at this)

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Jan 1, 2020

Thank you, Sander. I'm thinking of creating some content about plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants