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

feat: use native node loading for plugins + docs #638

Closed
wants to merge 2 commits into from

Conversation

coopernetes
Copy link
Contributor

@coopernetes coopernetes commented Jul 6, 2024

fixes #539

  • 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.

TODO:

  • Add test cases for PluginLoader
  • Decide whether plugin development docs should be on the doc site or fine to leave it in the repo. @JamieSlome or @maoo lmk your thoughts on the best place for this (both?)

- 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.
Copy link

netlify bot commented Jul 6, 2024

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 29fdf4d
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/66964cf16aecc10008202041
😎 Deploy Preview https://deploy-preview-638--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 57.89474% with 16 lines in your changes missing coverage. Please review.

Project coverage is 56.15%. Comparing base (9aaf885) to head (29fdf4d).

Files Patch % Lines
src/plugin.js 54.54% 15 Missing ⚠️
src/proxy/chain.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
- Coverage   57.39%   56.15%   -1.24%     
==========================================
  Files          46       45       -1     
  Lines        1582     1583       +1     
==========================================
- Hits          908      889      -19     
- Misses        674      694      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JamieSlome
Copy link
Member

@coopernetes - I'd say in both the README.md and docs site if possible. In the README.md, if we can compress into a few lines of how to create your first plugin, that would be fabulous 👍

In the docs site, we can go through more details for each step, the granular details etc. Thoughts?

@JamieSlome
Copy link
Member

JamieSlome commented Jul 15, 2024

Just had a fairly in-depth review of the pull request. Does the push plugins defined just add the plugins directly to the end of the array of the chain in order that they are configured in pushPlugins? Also, why pushPlugins? Should we not just expose plugins and specify the type of plugin from inside the plugin itself?

@maoo
Copy link
Member

maoo commented Jul 16, 2024

Great work @coopernetes, somehow you managed to wrap all of this with very few changes!

Re. the docs, I'd suggest to add the plugins/README.md as part of the website, then link the URL into the main README.md , to avoid defining the same docs in 2 different places.

I also agree with @JamieSlome re. pushPlugins VS plugins; the operation that gets implemented (in this case push) could be defined by the function signature that gets implemented (or how they get exported?)

I'll try to allocate some time this week to give this a go.

@maoo
Copy link
Member

maoo commented Jul 18, 2024

@coopernetes - I couldn't get it to work; I did some debugging, and it seems to me that the following block in plugin.js does not get executed:

    Promise.all(modulePromises).then((vals) => {
      const modules = vals;
      console.log(`Found ${modules.length} plugin modules`); // TODO: log.debug()
      const pluginObjPromises = [];
      for (const mod of modules) {
        pluginObjPromises.push(this._castToPluginObjects(mod));
      }
      Promise.all(pluginObjPromises).then((vals) => {
        for (const pluginObjs of vals) {
          this.plugins = this.plugins.concat(pluginObjs);
        }
        console.log(`Loaded ${this.plugins.length} plugins`); // TODO: log.debug()
      });
    });

even though the line modulePromises.push(this._loadPlugin(target)); gets correctly executed.

This is what I did to reproduce the issue:

{
  "project": "maoo",
  "name": "maven-tiles",
  "url": "https://github.com/maoo/maven-tiles.git"
}
  • Run git-proxy --config ./proxy.config.json
  • Open a new terminal
  • Checkout repo
cd /tmp
git clone http://localhost:8000/maoo/maven-tiles.git
cd maven-tiles
  • Edit README.md
  • git add README.md ; git commit -m 'test: small change' ; git push origin master
  • commented out proc.push.checkUserPushPermissionand proc.push.blockForAuth in chain.js, to make testing easier; I wonder if this list should be added as entries in proxy.config.js under pushPlugins.

Could it be something related with Promises?

@coopernetes
Copy link
Contributor Author

Does the push plugins defined just add the plugins directly to the end of the array of the chain in order that they are configured in pushPlugins?

Custom plugins are added after the parseAction but before any builtin actions that are part of the default chain.

See https://github.com/coopernetes/git-proxy/blob/chore/plugin-docs/src/proxy/chain.js#L56-L59

Also, why pushPlugins? Should we not just expose plugins and specify the type of plugin from inside the plugin itself?

We can have a generic set of "plugins" via configuration and attempt to do some funky casting....Note that this will become much cleaner with 2.0 + TypeScript (#425) since we can enforce stronger API contracts through TS. It's just not useful today with the JS code base. The thought was to be explicit about the plugin types so that when we load 3rd party modules, we will presume that plugin authors wrote the specific object (class) based on its functionality. In reality, a push and pull are virtually identical so we can update this in the new configuration & docs.

@coopernetes
Copy link
Contributor Author

@maoo I had this working when running from source. I'm not sure how the global install affects the plugin loading but I suspect it has to do with the "path" of git-proxy itself when it's sitting in your user's global npm cache in relation to the plugins. The load-plugin library relies on Node's builtin module resolution which looks at a few known locations.

I'll do some debugging on my side with a similar setup.

@coopernetes
Copy link
Contributor Author

Closing this to re-open on a separate fork. Our org is iterating on this a bit and testing internally first before it's suitable to merge.

@coopernetes coopernetes deleted the chore/plugin-docs branch August 15, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating your first plugin documentation
3 participants