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 Support - BETA #325

Merged
merged 30 commits into from
Nov 30, 2018
Merged

Plugin Support - BETA #325

merged 30 commits into from
Nov 30, 2018

Conversation

fishcharlie
Copy link
Member

@fishcharlie fishcharlie commented Mar 23, 2018

Plugin Support - BETA

WARNING: PLUGINS IS CURRENTLY IN BETA. THIS FUNCTIONALITY MIGHT CHANGE AT ANYTIME WITHOUT WARNING. DO NOT CONSIDER THIS FEATURE TO BE STABLE.

Summary

It's finally here (or coming soon, if this isn't merged in yet). Plugin Support is coming to Dynamoose. We are currently aiming to release this with Dynamoose version 1.2.0.

I'm creating this PR way too early. The reason for this is I'm trying to build this feature in the open to receive feedback and suggestions so the community can help build this and make it the best it can be.

Ideas/Structure

  • Plugins will be linked to models (possibly Dynamoose defaults as well in the future to link a plugin to all models, but will be integrated heavily into Dynamoose models)
  • It will be possible to register multiple plugins to one model
  • Plugins will watch for events to be "sent" or "emitted" from Dynamoose and can manipulate or change data
  • If * is passed into action, in the .on method it will match all emitted types. Or maybe we can make both action/type and stage optional.

Ideas we are exploring

  • Add support for plugins requiring a certain version of Dynamoose (and/or Node.js?)
  • Add feature to dynamoosejs.com to search plugins

Items marked with *** means that we are currently unsure how to implement this feature. Although it would be a cool feature, the details of the idea haven't been thought through to a level that makes implementation possible.

Potential Examples

Possible Plugin Implementation

module.exports = function(obj, options) {
	obj.setName(“My Plugin”); // required
	obj.setDescription(“”); // optional
	obj.on(‘init’, function() { // this will handle all stages related to init
		console.log(“Plugin registered”);
	});
	obj.on(‘scan’, ‘preRequest’, function(objB) { // this will handle only preRequest stages on the scan type
		console.log(“About to make request to DynamoDB”);
	});
	obj.on(‘scan’, ‘postRequest’, function(objB) { // this will handle only postRequest stages on the scan type, and will wait for promise to resolve before moving on
		return new Promise(function(resolve, reject) {
			resolve();
		});
	});
	return obj;
}

The obj.on method will add listeners that will be called when Dynamoose emits certain actions. It will accept 3 properties (action, [stage ,] func). action is a higher level stage that Dynamoose will emit. stage is a lower level stage that gives detail about where in the process Dynamoose is (for example right before AWS request, right after AWS request) all within a higher level action such as (.get, .scan, etc). If stage is not passed in, the function will be called for all emitted actions that match.

Adding Plugin to Model

var MyPlugin = require('ThePluginPackage');
var MyPluginB = require('ThePluginPackageB');
var Model = dynamoose.model('Puppy', {
    id: {
        type: Number,
        validate: function(v) {
            return v > 0;
        }
    },
    name: String,
    owner: String,
    age: {
        type: Number
    }
});
Model.plugin(MyPlugin); // this plugin will always take priority over the plugin below and be run first
Model.plugin(MyPluginB, {username: 'test', password: 'test'}); // this plugin will always take priority second and will be run after the first plugin, this plugin also passes options into the plugin

module.exports = Model;

Potential actions/types to emit

  • Plugin Register (“plugin:register”) (**)
  • AWS Table creation (“table:create”)
  • Scan (“model:scan”) (**)
  • Query (“model:query”) (**)
  • Save (“newmodel:save”) (this looks to be put)
  • Put (“model:put”) (**)
  • BatchPut (“model:batchput”) (might consider later)
  • Create (“model:create”) (might consider later)
  • Get (“model:get”) (**)
  • Populate (“model:populate”) (might consider later)
  • New Model Item (“newmodel:new”) (might consider later)
  • Update ("model:update")

** indicates an action that has been fully implemented.

Potential stages to emit

Stages (examples, depends on method):

  • Right when call is first made (right when .save call is made)
  • After Dynamoose has run the Dynamoify function
  • Right before call will be made to

Use Cases we need to support

Todo Items

  • Build some example/test plugins
  • Add support for adding global plugins (to all models)

Questions

  • Is plugin compatibility a concern? And if so, should/can we do anything about it?
  • Is having too many plugins/listeners on a model a concern? Any ideas on how to increase performance or is this a non issue?

Other Notes

  • I will be updating this PR with more detail as I work on it more
  • Feel free to comment or contact me if you have suggestions, questions, ideas, etc.
  • Close Support for plugins #18

lib/Scan.js Outdated
debug('scan request', scanOneReq);
Model.$__.base.ddb().scan(scanOneReq, function(err, data) {
Model._emit('model:scan', 'request:post', {event: {scan: this, callback: next, data: data, error: err}, actions: {updateError: function(error) {err = error;}, updateData: function(myData) {data = myData}}});
Copy link
Member Author

Choose a reason for hiding this comment

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

Concerned about async issues with some of these action functions

@fishcharlie fishcharlie mentioned this pull request Apr 3, 2018
@fishcharlie
Copy link
Member Author

fishcharlie commented Nov 2, 2018

For the request that @mogwai had regarding async support and exiting early. Please see this branch.

Things still to do on that branch:

  • Write documentation (DONE)

@fishcharlie
Copy link
Member Author

@mogwai You want to pull down the branch I mentioned in #325 (comment) and see if you can create the plugin you were looking at and give me some more feedback?

@mogwai
Copy link
Contributor

mogwai commented Nov 17, 2018

Yeah @fishcharlie I'll give it a go this week

@mogwai
Copy link
Contributor

mogwai commented Nov 27, 2018

Hi Charlie, I'm looking into your plugins-async branch. I can see that a caching plugin could now be created using the deferredMain promise and preventing the continued execution by returning false to continue.

My concern is that there is a possibility of Q.defer being removed soon? If so it might affect the way the plugin will work?

@fishcharlie
Copy link
Member Author

fishcharlie commented Nov 27, 2018

@mogwai PR #456 should not be a breaking change. If you return or use a promise in your plugin, the code I wrote should just await on that promise. I don't think that should effect how promises work.

If you want to use Q in your plugin, that should probably be fine so long as you include it in your package.json as a dependancy. From my understanding Q works with the same stuff as JavaScript promises. Like async/await. So if you return a Q promise, our code should be able to handle it. Or of course you could just use native JavaScript promises.

Hopefully this answers your question. Let me know if it doesn't tho. I really look forward to seeing what you come up with!!!

@mogwai
Copy link
Contributor

mogwai commented Nov 27, 2018

Well thats another thing on its own. I don't think the library should force the user to use a promise. It should check whether the return value is a promise and await it if so. Other plugins may not need to return a promise and will be forced to return Promise.resolve({}) or the library will throw an error. What are you thoughts?

I suggest changing the Plugin.js to:

let result = listener.emit(type, stage, obj);
result = result.then ? await result : result;

In the scanOne() function, you call ..._emit('model:scan', 'request:pre'... listeners and you pass the deferredMain which is a Q promise. I'm guessing you've done this so a plugin can return something to the user, overwriting the methods original functionality. Is this correct?

@fishcharlie
Copy link
Member Author

@mogwai If a plug-in returns a non promise that should be perfectly fine. I’m pretty sure await will handle that gracefully without any additional logic on our end.

I could be TOTALLY wrong tho.

@mogwai
Copy link
Contributor

mogwai commented Nov 27, 2018

Your right :) ignore that comment

@fishcharlie
Copy link
Member Author

I have updated the documentation in my pluginsupport-async branch. I will be merging that branch into this PR soon (might wait for @mogwai's input first tho).

I'm aiming to get this PR merged in before the next version (#464). Keep in mind this will just be a beta version and not considered stable for that release. The goal will be to gain more input and feedback from developers to solidify it in a future release.

@coveralls
Copy link

coveralls commented Nov 28, 2018

Pull Request Test Coverage Report for Build 735

  • 121 of 129 (93.8%) changed or added relevant lines in 5 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 85.152%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/Scan.js 11 12 91.67%
lib/Plugin.js 45 48 93.75%
lib/Model.js 41 45 91.11%
Files with Coverage Reduction New Missed Lines %
lib/Table.js 2 76.76%
lib/Model.js 6 83.51%
Totals Coverage Status
Change from base Build 700: -0.03%
Covered Lines: 1985
Relevant Lines: 2257

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 715

  • 90 of 94 (95.74%) changed or added relevant lines in 5 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 85.398%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/Model.js 23 24 95.83%
lib/Plugin.js 42 45 93.33%
Files with Coverage Reduction New Missed Lines %
lib/Table.js 2 76.76%
Totals Coverage Status
Change from base Build 700: 0.2%
Covered Lines: 1969
Relevant Lines: 2232

💛 - Coveralls

@fishcharlie
Copy link
Member Author

I have just merged the async branch into this PR.

@fishcharlie fishcharlie merged commit a591749 into dynamoose:master Nov 30, 2018
@fishcharlie fishcharlie deleted the pluginsupport branch November 30, 2018 20:41
@fishcharlie fishcharlie mentioned this pull request Feb 6, 2019
12 tasks
@fishcharlie fishcharlie mentioned this pull request May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for plugins
4 participants