Review response

plugin/src/main/java/io/deephaven/plugin/Plugin.java

@@ -10,6 +10,7 @@
  * A plugin is a structured extension point for user-definable behavior.
  *
  * @see ObjectType
+ * @see JsPlugin
  */
 public interface Plugin extends Registration {
 

plugin/src/main/java/io/deephaven/plugin/js/JsPlugin.java

@@ -14,9 +14,37 @@
 import java.nio.file.Path;
 
 /**
- * A JS plugin is a {@link Plugin} that allows adding javascript code under the server's URL path "js-plugins/". See
- * <a href="https://github.com/deephaven/deephaven-plugins#js-plugins">deephaven-plugins#js-plugins</a> for more details
- * about the underlying construction for JS plugins.
+ * A JS plugin is a {@link Plugin} that allows for custom javascript and related content to be served, see
+ * {@link io.deephaven.plugin.js}.
+ *
+ * <p>
+ * For example, if the following JS plugin was the only JS plugin installed
+ *
+ * <pre>
+ * JsPlugin.builder()
+ *         .name("foo")
+ *         .version("1.0.0")
+ *         .main(Path.of("dist/index.js"))
+ *         .path(Path.of("/path-to/my-plugin"))
+ *         .build()
+ * </pre>
+ *
+ * the manifest served at "js-plugins/manifest.json" would be equivalent to
+ *
+ * <pre>
+ * {
+ *   "plugins": [
+ *     {
+ *       "name": "foo",
+ *       "version": "1.0.0",
+ *       "main": "dist/index.js"
+ *     }
+ *   ]
+ * }
+ * </pre>
+ *
+ * and the file "/path-to/my-plugin/dist/index.js" would be served at "js-plugins/foo/dist/index.js". All other files of
+ * the form "/path-to/my-plugin/{somePath}" will be served at "js-plugins/foo/{somePath}".
  */
 @Immutable
 @BuildableStyle
@@ -47,21 +75,23 @@ public static Builder builder() {
      * ({@code Files.isRegularFile(root().resolve(main()))}) and must be included in {@link #paths()}. Will be included
      * as the "main" field for the manifest entry in "js-plugins/manifest.json".
      *
-     *
      * @return the main JS file path
      */
     public abstract Path main();
 
     /**
-     * The directory path of the resources to serve. The path must exist ({@code Files.isDirectory(path())}).
+     * The directory path of the resources to serve. The resources will be served via the URL path
+     * "js-plugins/{name}/{relativeToPath}". The path must exist ({@code Files.isDirectory(path())}).
      *
      * @return the path
      */
     public abstract Path path();
 
     /**
-     * The paths to serve, specified relative to {@link #path()}. The resources will be served via the URL path
-     * "js-plugins/{name}/{relativePath}". By default, is {@link Paths#all()}.
+     * The subset of resources from {@link #path()} to serve. Production installations should preferably be packaged
+     * with the exact resources necessary (and thus served with {@link Paths#all()}). During development, other subsets
+     * may be useful if {@link #path()} contains content unrelated to the JS content. By default, is
+     * {@link Paths#all()}.
      *
      * @return the paths
      */

plugin/src/main/java/io/deephaven/plugin/js/Paths.java

@@ -5,6 +5,9 @@
 
 import java.nio.file.Path;
 
+/**
+ * The subset of paths to serve, see {@link JsPlugin#paths()}.
+ */
 public interface Paths {
 
     /**

plugin/src/main/java/io/deephaven/plugin/js/package-info.java

@@ -0,0 +1,19 @@
+/**
+ * Copyright (c) 2016-2023 Deephaven Data Labs and Patent Pending
+ */
+
+/**
+ * The Deephaven server supports {@link io.deephaven.plugin.js.JsPlugin JS plugins} which allow custom javascript (and
+ * related content) to be served under the HTTP path "js-plugins/".
+ *
+ * <p>
+ * A "js-plugins/manifest.json" is served that allows clients to discover what JS plugins are installed. This will be a
+ * JSON object, and will have a "plugins" array, with object elements that have a "name", "version", and "main". All
+ * files served via a specific plugin will be accessed under "js-plugins/{name}/". The main entry file for a plugin will
+ * be accessed at "js-plugins/{name}/{main}". The "version" is currently for informational purposes only. For more
+ * information on the structure of the ...
+ *
+ * @see <a href="https://github.com/deephaven/deephaven-plugins">deephaven-plugins</a> for Deephaven-maintained JS
+ *      plugins
+ */
+package io.deephaven.plugin.js;

server/src/main/java/io/deephaven/server/plugin/js/JsPluginConfigDirRegistration.java

@@ -45,8 +45,7 @@ public interface Module {
     public void registerInto(Callback callback) {
         // <configDir>/js-plugins/ (manifest root)
         final Path manifestRoot = ConfigDir.get()
-                .map(JsPluginConfigDirRegistration::resolveJsPlugins)
-                .map(JsPluginConfigDirRegistration::resolveManifest)
+                .map(p -> p.resolve(JS_PLUGINS).resolve(MANIFEST_JSON))
                 .filter(Files::exists)
                 .map(Path::getParent)
                 .orElse(null);
@@ -63,12 +62,4 @@ public void registerInto(Callback callback) {
             callback.register(plugin);
         }
     }
-
-    private static Path resolveJsPlugins(Path p) {
-        return p.resolve(JS_PLUGINS);
-    }
-
-    private static Path resolveManifest(Path p) {
-        return p.resolve(MANIFEST_JSON);
-    }
 }

server/src/main/java/io/deephaven/server/plugin/js/JsPluginFromNpmPackage.java

@@ -9,53 +9,23 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.List;
 
 class JsPluginFromNpmPackage {
 
-    // https://docs.npmjs.com/cli/v6/configuring-npm/package-json#files
-    private static final List<String> INCLUDED = List.of(
-            "package.json",
-            "README",
-            "README.md",
-            "LICENSE",
-            "LICENSE.md",
-            "LICENCE",
-            "LICENCE.md",
-            "CHANGES",
-            "CHANGES.md",
-            "CHANGELOG",
-            "CHANGELOG.md",
-            "HISTORY",
-            "HISTORY.md",
-            "NOTICE",
-            "NOTICE.md");
-
     static JsPlugin of(Path packageRoot) throws IOException {
         final Path packageJsonPath = packageRoot.resolve(JsPluginNpmPackageRegistration.PACKAGE_JSON);
         final NpmPackage packageJson = NpmPackage.read(packageJsonPath);
         final Path main = packageRoot.relativize(packageRoot.resolve(packageJson.main()));
         final Paths paths;
         if (main.getNameCount() > 1) {
-            // We're requiring that all of the necessary files (besides INCLUDED) be under the top-level directory
-            // as sourced from package.json/main. Typically, "dist" or "build".
-            // To be technically correct, we'd also need to read .npmignore and fully parse package.json/files, and
-            // additionally ensure that any jetty routing matched up precisely - this is harder to do in general, so we
-            // are making these simplifying assumptions.
+            // We're requiring that all of the necessary files to serve be under the top-level directory as sourced from
+            // package.json/main. For example, "build/index.js" -> "build", "dist/bundle/index.js" -> "dist". This
+            // supports development use cases where the top-level directory may be interspersed with unrelated
+            // development files (node_modules, .git, etc).
             //
-            // Note: this logic really only comes into play or development use cases where plugins are configured via
+            // Note: this logic only comes into play for development use cases where plugins are configured via
             // deephaven.jsPlugins.myPlugin=/path/to/my/js
-            // For python delivering JS plugins, they'll be able to explicitly configure JsPlugin as appropriate; that
-            // said, we may want to prefer they create JsPlugin via this code path for consistency purposes.
-            final Path topLevelDirectory = main.subpath(0, 1);
-            final List<Path> prefixes = new ArrayList<>(INCLUDED.size() + 2);
-            prefixes.add(topLevelDirectory);
-            prefixes.add(packageRoot.relativize(packageJsonPath));
-            for (String included : INCLUDED) {
-                prefixes.add(packageRoot.relativize(packageRoot.resolve(included)));
-            }
-            paths = Paths.ofPrefixes(prefixes);
+            paths = Paths.ofPrefixes(main.subpath(0, 1));
         } else {
             paths = Paths.all();
         }