feat: expose plugin extension points into main package, developer docs + sample #713
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #713 +/- ##
==========================================
+ Coverage 57.39% 57.52% +0.13%
==========================================
Files 46 46
Lines 1582 1627 +45
==========================================
+ Hits 908 936 +28
- Misses 674 691 +17 ☔ View full report in Codecov by Sentry. |
| * @return {boolean} - True if the object or any of its prototypes has the 'isGitProxyPlugin' property set to true, false otherwise. | ||
| */ | ||
| function isCompatiblePlugin(obj, propertyName = 'isGitProxyPlugin') { | ||
| while (obj != null) { |
There was a problem hiding this comment.
This could use a comment. It wasn't immediately clear to me what the while was doing.
(Then again, I'm not a JS dev.)
There was a problem hiding this comment.
I believe this is left over from a previous iteration that checked objects recursively. It can likely be removed.
There was a problem hiding this comment.
Looking at this further, in the code we are traversing the prototype chain until we get a null result. If none of the prototypes have the expected property set (ie. it's an object that isn't a subclass of ProxyPlugin & its derivatives), it'll eventually return false after exhausting the traversal.
I will add a small comment for this.
| @@ -23,6 +25,10 @@ proxyApp.use(bodyParser.raw(options)); | |||
| proxyApp.use('/', router); | |||
|
|
|||
| const start = async () => { | |||
| const plugins = config.getPlugins(); | |||
| const pluginLoader = await createLoader(plugins); | |||
| await pluginLoader.load; | |||
There was a problem hiding this comment.
This is a nitpick, but it feels weird for something async to be a property and not a method.
There was a problem hiding this comment.
Some of this logic can be simplified. We do need to block at certain points to ensure that the PluginLoader has parsed all the modules and added it to its list of plugin objects before proceeding with the rest of the app startup.
If this were a method, we would need to do some sort of while { } loop with a predefined timeout to "block" on completion. It's more straight forward to simply await on a Promise that will eventually resolve.
Open to suggestions to improve this section but this code is working well enough in a live environment so I'm inclined to retain it.
|
Thanks @rgmz for the review! Can someone look at his comments? And since we're there, you may probably want to update the branch with latest changes from I read through the PR, code looks clean and docs seems to be solid; sadly - with OSFF in 2 weeks - it's hard for me to find time to test https://deploy-preview-713--endearing-brigadeiros-63f9d0.netlify.app/docs/development/plugins , so if anyone would be interested to go through these docs and test it locally, it would be great! Sections that are IMO particularly interesting are https://deploy-preview-713--endearing-brigadeiros-63f9d0.netlify.app/docs/development/plugins#via-npm-packages and https://deploy-preview-713--endearing-brigadeiros-63f9d0.netlify.app/docs/development/plugins#developing-new-plugins FYI - @vaibssingh @divinetettey @bill-n |
Any chance we can bump up the test coverage before merging? |
…s + sample - update PluginLoader to use load-plugin's native Node module resolution. this library already handles loading of files vs modules in node_modules/ so let's use that directly - add configuration-based plugin loading and deprecate environment variables - new documentation page for plugin development. loading, writing, etc. - add PullActionPlugin + unify config on just "plugins" - add explicit exports to make importing @finos/git-proxy more straight forward for plugins In addition, this commit adds both CommonJS and ES module examples of plugins and refactors them into a more reflective package. It also includes updates to the site documentation, specifically in the Development section, which now includes details about plugins and contribution guidelines. - fix issue with json-schema-for-humans producing bad output due to post-processing (script step no longer required) - docs: add section on docs on how to update the config schema + re-generate reference doc - fix: don't package website with git-proxy, update .npmignore on npm pack/publish
- remove actions which are used in internal fork from chain - add default config value for plugins (an empty array)
coopernetes
force-pushed
the
feat/new-plugin-system
branch
from
007b5d7
to
6d539de
Compare
I can add additional tests if necessary. Note that these code paths have not had previous tests so some additional refactoring may be required. Mind if we incorporate this in a follow-up PR? It's more important to give developers adequate docs for plugin development ahead of improving test coverage for existing code that is known to be working. I've added tests for the most critical new code paths. This PR adds e2e tests for plugins to be installed & consumed which do validate these missing code paths - it's just not reflected in coverage. |
Happy 🤝 @coopernetes - once you've resolved all remaining conversations, happy for you to proceed and we can merge, and release 🎉 I'll get the LinkedIn juices flowing and put together a social media post as well. |
|
Honestly can't wait to have the discussion about community plugins ❤️ |
| * @return {PluginLoader} | ||
| */ | ||
| const createLoader = async (targets) => { | ||
| const loadTargets = targets; |
There was a problem hiding this comment.
minor nitpick but do we need to reassign the targets to loadTargets here? Can we not directly pass it on PluginLoader?
There was a problem hiding this comment.
Nope, the reassignment is extraneous - this is left over from a previous implementation so it can be removed. I'll clean this up.
handles loading of files vs modules in node_modules/ so let's use that directly
In addition, this commit adds both CommonJS and ES module examples of plugins and refactors them
into a more reflective package. It also includes updates to the site documentation, specifically
in the Development section, which now includes details about plugins and contribution guidelines.
no longer required)
Side note:
The version was bumped fromreverted this change, now using the version straight from main.1.3.4to1.3.5-alpha.0. This was necessary for validating the new package behaviour on a version that was distinct from the latest version of the public@finos/git-proxypackage. This can be rolled into a cumulative release instead of an "alpha" test release.