fix: DH-23199: Propagate loader details from package.json to the manifest.json (#8299) - #8446
Open
mofojed wants to merge 1 commit into
Open
Conversation
…nifest.json (deephaven#8299) - This is an attempt to port over the behaviour we implemented in Enterprise: deephaven-ent/iris#4887 and deephaven-ent/iris#4914 - Working on DH-16228, the UI now loads plugins directly from the worker. However, the `loader` field is currently not being propagated through to the `manifest.json` that is populated by the worker, which breaks the `pivot-builder` plugin (the `loader` field provides details that it depends on the `pivot` plugin) - Tested on https://bender-dh-16228.int.illumon.com:8123/iriside/ - Core+ BETA worker kind is using this --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Jianfeng Mao <4297243+jmao-denver@users.noreply.github.com> (cherry picked from commit 893188e)
mofojed
requested review from
chipkent,
cpwright,
devinrsmith,
jcferretti,
jmao-denver and
rcaudy
as code owners
September 1, 2026 21:15
Contributor
No docs changes detected for 45fc06a |
Contributor
There was a problem hiding this comment.
Pull request overview
Propagates JS plugin loader metadata from package.json through plugin registration into generated manifests.
Changes:
- Adds loader support across Java and Python plugin paths.
- Updates plugin packaging and integration tests.
- Refreshes related Python dependencies and container images.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
server/.../NpmPackage.java |
Parses loader metadata. |
server/.../JsPluginsFromManifest.java |
Restores loaders from manifests. |
server/.../JsPluginManifestEntry.java |
Models manifest loader data. |
server/.../JsPluginFromNpmPackage.java |
Transfers package loaders. |
server/jetty/.../manifest.json |
Adds a loader fixture. |
server/jetty/.../example1/package.json |
Adds package loader data. |
server/jetty/.../Example123Registration.java |
Registers a loader fixture. |
server/jetty/.../JettyFlightRoundTripTest.java |
Verifies manifest propagation. |
py/server/tests/test_plugin_js.py |
Tests Python plugin conversion. |
py/server/tests/test_jcompat.py |
Tests recursive JSON conversion. |
py/server/deephaven/jcompat.py |
Adds Java JSON conversion. |
py/server/deephaven/__init__.py |
Extends the package namespace. |
py/server/deephaven_internal/plugin/js/__init__.py |
Transfers Python loaders. |
plugin/.../JsPlugin.java |
Exposes the loader API. |
docker/.../pack-plugins.sh |
Preserves loaders in packaged manifests. |
docker/server/.../requirements.txt |
Refreshes Netty dependencies. |
docker/server-jetty/.../requirements.txt |
Refreshes Jetty dependencies. |
docker/registry/slim-base/gradle.properties |
Updates the slim image digest. |
docker/registry/server-base/gradle.properties |
Updates the server image digest. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * | ||
| * @return the loader configuration | ||
| */ | ||
| public abstract Optional<Object> loader(); |
Member
There was a problem hiding this comment.
JsPlugin is part of a JAVA_PUBLIC module and may already have third-party subclasses
This is not true; it's Immutable, and the only impls are owned by Deephaven.
Comment on lines
32
to
+36
| public static JsPluginManifestEntry of( | ||
| @JsonProperty(value = NAME, required = true) String name, | ||
| @JsonProperty(value = VERSION, required = true) String version, | ||
| @JsonProperty(value = MAIN, required = true) String main) { | ||
| return ImmutableJsPluginManifestEntry.of(name, version, main); | ||
| @JsonProperty(value = MAIN, required = true) String main, | ||
| @JsonProperty(value = LOADER) Object loader) { |
mofojed
added
NoDocumentationNeeded
NoReleaseNotesNeeded
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
loaderfield is currently not being propagated through to themanifest.jsonthat is populated by the worker, which breaks thepivot-builderplugin (theloaderfield provides details that it depends on thepivotplugin)(cherry picked from commit 893188e)