Add concrete JsPlugin #4925

devinrsmith · 2023-12-07T22:26:28Z

This PR gives a concrete structure to JsPlugin; name, version, main, path, and paths. All existing means of configuration for JS plugins delegates to JsPlugin. Java jars can now provide JS plugins with this change as well.

This is also a partial improvement for #4817 (JS Plugins development is slow), in that JsPlugin paths is a mean to reduce the scope of files that needs to be copied. This is not a full solution (which may involve routing paths in Jetty that lead directly to the resources as configured via the JS plugins).

For NPM packages, this PR takes a practical approach as opposed to a "perfect" approach; we are requiring that all "real" files be included under the top-level directory as specified by package.json/main. For example, if main is "build/index.js", we'll include everything under "build/". If main is "dist/bundle/index.js", we'll include everything under "dist/".

The inclusion of the matching python structures will follow shortly, either as additional commits to this PR, or as follow-up PR.

Adds testing, fixes #4893

This PR gives a concrete structure to JsPlugin; name, version, main, root, and paths. All existing means of configuration for JS plugins delegates to JsPlugin. Java jars can now provide JS plugins with this change as well. This is also a _partial_ improvement for deephaven#4817 (JS Plugins development is slow), in that JsPlugin paths is a mean to reduce the scope of files that needs to be copied. This is _not_ a full solution (which may involve routing paths in Jetty that lead directly to the resources as configured via the JS plugins). For NPM packages, this PR takes a practical approach as opposed to a "perfect" approach; we are requiring that all "real" files be included under the top-level directory as specified by package.json/main. For example, if main is "build/index.js", we'll include everything under "build/". If main is "dist/bundle/index.js", we'll include everything under "dist/". The inclusion of the matching python structures will follow shortly, either as additional commits to this PR, or as follow-up PR. Adds testing, fixes deephaven#4893

devinrsmith · 2023-12-08T18:23:55Z

I've added the deephaven-core glue layer for the existing (but never used) python type deephaven.plugin.js.JsPlugin. I think we may want to update that type w/ some new naming and docs, but the existing structure will probably look very similar.

niloc132 · 2023-12-08T20:12:20Z

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

+
+import java.nio.file.PathMatcher;
+
+interface PathsInternal extends Paths, PathMatcher {


Unsolicited advice, and unimportant for this iteration, but might be handy if considering using PathMatcher more generally: FileSystems.getDefault().getPathMatcher(...) supports glob: patterns, but if you do, take care to note that **/* does not match files in the current dir (that is, something which * would match).

Yeah - I considered making PathMatcher the public API, but I don't know what sort of interface limitations we might wish we had imposed if we want an "easy" route-based integration w/ jetty.

niloc132 · 2023-12-08T20:25:02Z

py/server/deephaven_internal/plugin/js/__init__.py

+
+
+def to_j_js_plugin(js_plugin: JsPlugin) -> jpy.JType:
+    # TODO: update deephaven-plugin JsPlugin, probably rename distribution_path to root (to match JsPlugin?)


(if my comment isnt already outdated...) i'm guessing this is pending in the deephaven-plugin repo?

I don't have any code yet; but it's simple once we decide the naming we are OK w/. deephaven/deephaven-plugin#15

niloc132 · 2023-12-08T20:26:01Z

py/server/deephaven_internal/plugin/js/__init__.py

+        # Note: this only works for "normally" distributed / installed packages.
+        # In cases where the package is distributed as zip, we'll need additional logic to create a zip resource path,
+        # or copy the data into a temporary directory.
+        # See note about zip from https://docs.python.org/3.11/library/importlib.resources.html#importlib.resources.path


can we detect and throw/warn about this case? @jmao-denver is this case at all common?

also I know that https://peps.python.org/pep-0441/ exists, is supported, but probably not by us

I think it's pretty uncommon; that said, I suspect there is a way to use create proper zip URI/Path if it comes down to it. I'm guessing we can look at the return type to figure out. Somewhat relevant discussion around this topic, python/cpython#99818

server/jetty/src/main/java/io/deephaven/server/jetty/CopyHelper.java

server/jetty/src/main/java/io/deephaven/server/jetty/JsPlugins.java

niloc132 · 2023-12-08T20:33:53Z

server/jetty/src/main/java/io/deephaven/server/jetty/JsPluginsZipFilesystem.java

+            // If listing and traversing the contents of development directories (and skipping the copy) becomes
+            // too expensive, we can add logic here wrt PathsInternal/PathsPrefix to specify a dirMatcher. Or,
+            // properly route directly from the filesystem via Jetty.
+            CopyHelper.copyRecursive(plugin.root(), dstPath, d -> true, pathMatcher);


only usage of dirMatcher, and d -> true here as a constant, inline into CopyHelper.copyRecursive (esp since that is package protected and will never be called by user code)?

Will do - I think there's a chance we'll end up using dirMatcher in the future (if, for some reason we are unable to use a jetty route-based approach), so I'll keep the underlying impl.

niloc132 · 2023-12-08T20:37:47Z

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

+    @JsonProperty(MAIN)
+    public abstract String main();
+
+    // Note: do we want "files" here, and actually try to parse it? Or, "deephaven" object that more closely matches


tag with a follow-up ticket?

I'll update the comment and link to #4817, which I think is sufficient.

In addition, updates JsPlugin documentation to be consistent with deephaven/deephaven-core#4925 Fixes deephaven#15

In addition, updates JsPlugin documentation to be more consistent with deephaven/deephaven-core#4925 Fixes #15

This bump is to minimize the bump needed wrt #4925

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

niloc132 · 2023-12-14T02:06:36Z

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

+
+import java.nio.file.Path;
+
+public interface Paths {


We might consider a different class name here - when dealing with java.nio.file.Path instances, it is common to use java.nio.file.Paths to create them.

Also, javadoc

I'm not sure how to best rename it if necessary - I'm personally less offended by names that overlap in these regards, although I know others disagree. At least java.nio.file.Paths is somewhat discouraged in their javadoc:

* @apiNote * It is recommended to obtain a {@code Path} via the {@code Path.of} methods * instead of via the {@code get} methods defined in this class as this class * may be deprecated in a future release.

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

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

jmao-denver

Python changes LGTM

deephaven-internal · 2023-12-14T21:16:22Z

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3550
Reference: https://github.com/deephaven/deephaven.io/issues/3549

mofojed · 2023-12-14T21:09:48Z

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

+ * 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}".


Seems kind of redundant - why not just say "All files of from "/path-to/my-plugin/{somePath}" will be served at "js-plugins/foo/{somePath}"? The main by definition is going to be relative to that path, ergo must always be served.

mofojed · 2023-12-14T21:11:14Z

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

+    }
+
+    /**
+     * The JS plugin name. The JS plugin contents will be served via the URL path "js-plugins/{name}/", as well as


Should note that name can have a / in it for subfolders. Likely important to note that we do not want to URL encode that /, we want it to actually be at js-plugins/@scope/name, not js-plugins/@scope%2Fname

devinrsmith added DocumentationNeeded plug-ins javascript Pull requests that update Javascript code ReleaseNotesNeeded Release notes are needed labels Dec 7, 2023

devinrsmith added this to the November 2023 milestone Dec 7, 2023

devinrsmith requested review from niloc132 and mofojed December 7, 2023 22:26

devinrsmith self-assigned this Dec 7, 2023

devinrsmith added 2 commits December 7, 2023 14:34

cleanup

e3ccbc1

Add support for python JsPlugin

2fed742

devinrsmith requested review from chipkent, jmao-denver and rcaudy as code owners December 8, 2023 18:24

Rename enum INSTANCE to ALL for better toString()

bd6f098

This was referenced Dec 8, 2023

feat: Add plotly-express JsPlugin implementation and registration deephaven/deephaven-plugins#150

Merged

Finalize JsPlugin deephaven/deephaven-plugin#15

Closed

niloc132 reviewed Dec 9, 2023

View reviewed changes

Review responses

947a5ec

devinrsmith requested a review from niloc132 December 9, 2023 18:29

devinrsmith added a commit to devinrsmith/deephaven-plugin that referenced this pull request Dec 11, 2023

Updates JsPlugin distribution_path to root

59219ab

In addition, updates JsPlugin documentation to be consistent with deephaven/deephaven-core#4925 Fixes deephaven#15

devinrsmith mentioned this pull request Dec 11, 2023

Update JsPlugin for proper server-side support deephaven/deephaven-plugin#16

Merged

devinrsmith added 2 commits December 11, 2023 12:01

Make work w/ zip FS

24c6063

f

bcde5ff

devinrsmith added a commit to deephaven/deephaven-plugin that referenced this pull request Dec 13, 2023

Update JsPlugin for proper server-side support (#16)

1aed15b

In addition, updates JsPlugin documentation to be more consistent with deephaven/deephaven-core#4925 Fixes #15

devinrsmith mentioned this pull request Dec 13, 2023

Bump server-base images #4943

Merged

devinrsmith added a commit that referenced this pull request Dec 13, 2023

Bump server-base images (#4943)

ec1e808

This bump is to minimize the bump needed wrt #4925

devinrsmith added 2 commits December 13, 2023 11:04

Renames

3eb9c3d

Merge remote-tracking branch 'upstream/main' into js-plugins-v2

5132e10

devinrsmith added 2 commits December 13, 2023 11:11

Bump server base for deephaven-plugin==0.6.0

c23950f

Update to_j_js_plugin

0522799

devinrsmith requested a review from jcferretti as a code owner December 13, 2023 19:29

niloc132 reviewed Dec 14, 2023

View reviewed changes

Review response

60731f1

devinrsmith requested a review from niloc132 December 14, 2023 19:14

remove part

eece802

niloc132 previously approved these changes Dec 14, 2023

View reviewed changes

Fix tests after NPM change

d8fe591

devinrsmith dismissed niloc132’s stale review via d8fe591 December 14, 2023 20:09

niloc132 approved these changes Dec 14, 2023

View reviewed changes

jmao-denver approved these changes Dec 14, 2023

View reviewed changes

devinrsmith enabled auto-merge (squash) December 14, 2023 21:11

jcferretti approved these changes Dec 14, 2023

View reviewed changes

devinrsmith merged commit aadf4c8 into deephaven:main Dec 14, 2023
19 checks passed

devinrsmith deleted the js-plugins-v2 branch December 14, 2023 21:16

github-actions bot locked and limited conversation to collaborators Dec 14, 2023

mofojed reviewed Dec 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concrete JsPlugin #4925

Add concrete JsPlugin #4925

devinrsmith commented Dec 7, 2023 •

edited

Loading

devinrsmith commented Dec 8, 2023

niloc132 Dec 8, 2023

devinrsmith Dec 9, 2023

niloc132 Dec 8, 2023

devinrsmith Dec 9, 2023

niloc132 Dec 8, 2023

devinrsmith Dec 9, 2023

niloc132 Dec 8, 2023

devinrsmith Dec 9, 2023

niloc132 Dec 8, 2023

devinrsmith Dec 9, 2023

niloc132 Dec 14, 2023

devinrsmith Dec 14, 2023

jmao-denver left a comment

deephaven-internal commented Dec 14, 2023

mofojed Dec 14, 2023

mofojed Dec 14, 2023

devinrsmith Dec 14, 2023


		import java.nio.file.PathMatcher;

		interface PathsInternal extends Paths, PathMatcher {



		def to_j_js_plugin(js_plugin: JsPlugin) -> jpy.JType:
		# TODO: update deephaven-plugin JsPlugin, probably rename distribution_path to root (to match JsPlugin?)

		* 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}".

Add concrete JsPlugin #4925

Add concrete JsPlugin #4925

Conversation

devinrsmith commented Dec 7, 2023 • edited Loading

devinrsmith commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmao-denver left a comment

Choose a reason for hiding this comment

deephaven-internal commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith commented Dec 7, 2023 •

edited

Loading