[DAEMON-215] Core: Include global node_modules directory in the plugins

GitHub Issue	n/a
Type	Improvement
Priority	None
Status	Resolved
Resolution	Fixed
Resolution Date	2018-01-12T17:29:23.000+0000
Affected Version/s	n/a
Fix Version/s	Appc Daemon 1.1.0
Components	appcd-core
Labels	n/a
Reporter	Ewan Harris
Assignee	Ewan Harris
Created	2018-01-09T12:16:00.000+0000
Updated	2018-01-12T17:29:23.000+0000

Description

Currently the daemon core adds the plugin directories under appcd-default-plugins and ~/.appcelerator/appcd/plugins to the PluginManager on start. I would like to propose that we also add the global node_modules directory to this. The benefits I see are - Plugins not distributed with the daemon do not need to a) tell users to npm install with a --prefix flag b) have to have a post-install flag to ensure that they get picked up by the daemon (most titanium cli plugins I've seen do this) - npm install -g becomes the easy way of getting plugins, we easily get the discoverability of the npm registry The drawbacks I see are - It's simple to expand this idea into allowing us to pickup local node_modules (i.e in a titanium application), while this works in the realm of a once and done process like the titanium cli I don't fully understand how we can support the concept of local plugins in the daemon. - We could potentially majorly increase the number of fswatchers we setup when reading in all the global node modules I foresee an implementation that is structured as follows * Add the global node_modules dir to the paths array of the PluginManager creation in appcd-core * In appcd-plugin/src/plugin.js, throw an error for any package.json's we read in that do _not_ contain an appcd field ** This is a changeable requirement but I believe that unless there is a way to tell what is an appc-daemon plugin then we will register _every_ folder under node_modules, other tools do this by using TOOL_NAME-plugin-X, but seeing as we're reading in the package.json anyway I don't see why we cant add the validation there to lift some restrictions on the naming conventions.

Comments

Ewan Harris 2018-01-09

[~cbarber] FYI, I finally got round to writing this up, it seems easily doable but easily footgun-able

Chris Barber 2018-01-11

I'm not a huge fan of this, but I can see the usefulness. As you suggested, we have to enforce that plugin's package.json's MUST contain an appcd section. If we don't then we don't have any way to distinguish appcd plugins from globally installed node modules. For fun, I added /usr/local/lib/node_modules to the plugin init and here's what it discovered:

   Plugin                   Version   Type      Path                                                 Node.js    Status                         Active/Total Requests
   acs                      1.2.0     external  /usr/local/lib/node_modules/acs                      >= 0.6.0   Inactive                       0 / 0
   alloy                    1.10.3    external  /Users/chris/appc/alloy                              >=0.10     Inactive                       0 / 0
   android                  1.1.0     external  /Users/chris/appc/appcd-plugin-android               8.9.1      Inactive                       0 / 0
   appcelerator             4.2.11-2  external  /usr/local/lib/node_modules/appcelerator             >= 4.0     Inactive                       0 / 0
   eslint                   4.3.0     external  /usr/local/lib/node_modules/eslint                   >=4        Inactive                       0 / 0
   eslint-config-axway      2.0.7     external  /Users/chris/appc/eslint-config-axway                8.9.1      Inactive                       0 / 0
   genymotion               1.1.0     external  /Users/chris/appc/appcd-plugin-genymotion            8.9.1      Inactive                       0 / 0
   gulp                     3.9.1     external  /usr/local/lib/node_modules/gulp                     >= 0.9     Inactive                       0 / 0
   hpm-cli                  1.4.0     external  /usr/local/lib/node_modules/hpm-cli                  >=4        Inactive                       0 / 0
   ios                      1.1.0     external  /Users/chris/appc/appcd-plugin-ios                   8.9.1      Inactive                       0 / 0
   jdk                      1.1.0     external  /Users/chris/appc/appcd-plugin-jdk                   8.9.1      Inactive                       0 / 0
   lerna                    2.6.0     external  /usr/local/lib/node_modules/lerna                    >= 4       Inactive                       0 / 0
   mocha                    3.2.0     external  /usr/local/lib/node_modules/mocha                    >= 0.10.x  Inactive                       0 / 0
   node-gyp                 3.6.2     external  /usr/local/lib/node_modules/node-gyp                 >= 0.8.0   Inactive                       0 / 0
   node-inspector           0.12.8    external  /usr/local/lib/node_modules/node-inspector           >=0.8.0    Inactive                       0 / 0
   npm                      5.6.0     external  /usr/local/lib/node_modules/npm                      8.9.1      Inactive                       0 / 0
   npm-check-updates        2.14.0    external  /usr/local/lib/node_modules/npm-check-updates        >=0.12     Inactive                       0 / 0
   nsp                      2.6.2     external  /usr/local/lib/node_modules/nsp                      8.9.1      Inactive                       0 / 0
   retire                   1.4.0     external  /usr/local/lib/node_modules/retire                   >= 0.10.0  Inactive                       0 / 0
   sloc                     0.1.11    external  /usr/local/lib/node_modules/sloc                     8.9.1      Inactive                       0 / 0
   system-info              1.1.0     external  /Users/chris/appc/appcd-plugin-system-info           8.9.1      Inactive                       0 / 0
   titanium                 5.0.14    external  /Users/chris/appc/titanium                           >=4.0      Inactive                       0 / 0
   titanium-sdk             1.0.1     external  /Users/chris/appc/appcd-plugin-titanium-sdk          8.9.1      Inactive                       0 / 0
   windows                  1.1.0     external  /Users/chris/appc/appcd-plugin-windows               8.9.1      Unsupported platform "darwin"  0 / 0

So, enforcing an appcd section is a must. As for filtering based on the package name starting with appcd-plugin-, I don't love that. The plugin system used to automatically strip /^appcd-plugin-/ from the package name when determining the plugin name, but I ripped it out in favor of explicitly allowing the plugin to define it using appcd.name. The appcd-plugin-* naming convention was intended more for finding plugins in npm. As far as the fs watchers is concerned, I'm not worried abut it. It's not recursive and only adds a fs watcher for each directory in /usr/local/lib/node_modules which on my machine is only 34 directories. I did a test and created over 25k fs watchers on macOS without issue. Being that this feature would add about a half dozen lines of code, I'm find adding it to 1.1.0. We need to make sure there are no negative side effects.

Ewan Harris 2018-01-11

Yeah the filtering based of name was more of a if for some reason the requirement of appcd in package.json was shot down here's another way of figuring out what's valid. I'd like to try and understand the plugin stuff a little more so mind if I take this one on?
Chris Barber 2018-01-11

Be my guest. :)
Ewan Harris 2018-01-12

https://github.com/appcelerator/appc-daemon/pull/264
Ewan Harris 2018-01-12

Published a test plugin to npm if it's wanted for testing npm install -g appcd-plugin-ewan-test, endpoint is /ewan-test/latest/hello. Two points that are still possible todos here * Scoped packages don't get loaded * Yarn has it's own global modules directory so we might want to add that

JSON Source