fix: DH-23199: Propagate loader details from package.json to the manifest.json - #8299
fix: DH-23199: Propagate loader details from package.json to the manifest.json#8299mofojed wants to merge 6 commits into
loader details from package.json to the manifest.json#8299Conversation
No docs changes detected for a3c8476 |
devinrsmith
left a comment
devinrsmith
left a comment
There was a problem hiding this comment.
Bit out of the loop and confused about the overall goals. https://github.com/deephaven-ent/iris/pull/4914 does not shed any light on it...
| * | ||
| * @return the loader configuration | ||
| */ | ||
| public abstract Optional<Map<String, Object>> loader(); |
There was a problem hiding this comment.
This is sort-of a builder-anti-pattern; that is, using a Map or Collection w/ optional. The pattern is preferably to just have an empty map when there is no value.
| final ContentResponse manifestResponse = get(client, "js-plugins/manifest.json"); | ||
| assertOk(manifestResponse, "application/json", | ||
| "{\"plugins\":[{\"name\":\"@deephaven_test/example1\",\"version\":\"0.1.0\",\"main\":\"dist/index.js\"},{\"name\":\"@deephaven_test/example2\",\"version\":\"0.2.0\",\"main\":\"dist/index.js\"}]}"); | ||
| "{\"plugins\":[{\"name\":\"@deephaven_test/example1\",\"version\":\"0.1.0\",\"main\":\"dist/index.js\",\"loader\":{\"npm\":\"loader\"}},{\"name\":\"@deephaven_test/example2\",\"version\":\"0.2.0\",\"main\":\"dist/index.js\"}]}"); |
There was a problem hiding this comment.
This would be a good time to upgrade to Java 17 textblocks, which make it makes these sorts of strings much easier to read (no need to escape, can do multiple lines if necessary, etc).
| * | ||
| * @return the loader configuration | ||
| */ | ||
| public abstract Optional<Map<String, Object>> loader(); |
There was a problem hiding this comment.
Is the intention to support complex map values? Or, just strings? Or, is it supposed to be a Json value type?
There was a problem hiding this comment.
It's a JSON value type, e.g. https://github.com/deephaven/deephaven-plugins/blob/aaa4b0581b0e378acf13ee13e764e486671dea39/plugins/pivot-builder/src/js/package.json#L20
The goal is to have additional metadata provided by the package.json that is included in the manifest.json, such that we can read that before loading the plugins (as there's an order to which the plugins can be loaded in).
There was a problem hiding this comment.
Okay, so DH server doesn't care about the string keys at all? In that case, it should probably just be an Object if we are going to keep this design.
| @Nullable | ||
| @Parameter | ||
| @JsonProperty(LOADER) | ||
| public abstract Map<String, Object> loader(); |
There was a problem hiding this comment.
With this additional parameter, I would upgrade this object from "Simple" to "Buildable" style, remove Nullability.
| @SimpleStyle | ||
| abstract class NpmPackage { | ||
|
|
||
| static final String LOADER = "loader"; |
There was a problem hiding this comment.
I'm confused about this; is "loader" a standard field in package.json? Is this something we are adding to our plugin infra?
I don't see it https://docs.npmjs.com/cli/v12/configuring-npm/package-json.
There was a problem hiding this comment.
It is not a standard field. It's a custom field for our purposes.
mofojed
changed the title
loader details from package.json to the manifest.jsonloader details from package.json to the manifest.json
mofojed
added
NoDocumentationNeeded
NoReleaseNotesNeeded
Previously, the 'loader' field from a JS plugin's package.json was silently dropped and never appeared in the served js-plugins/manifest.json. - JsPlugin gains an optional loader() property/builder method. - NpmPackage/JsPluginFromNpmPackage read the 'loader' field from package.json (dev/npm-package registration path). - JsPluginManifestEntry/JsPluginsFromManifest read/write the 'loader' field to/from manifest.json (resource-path registration path). - JsPluginManifestEntry.from(JsPlugin) writes loader() when building the served manifest, covering both registration paths since all registered plugins funnel through it. - Python glue (to_j_js_plugin) duck-types a loader attribute on the Python JsPlugin and forwards it to the Java builder, so dynamically registered Python plugins (e.g. deephaven.ui) can also opt-in. - Updated JettyFlightRoundTripTest and JS plugin test fixtures to cover loader propagation across manifest, npm-package, and direct registration paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the manifest.json produced by pack-plugins.sh was built line-by-line via 'npm pkg get name version main', which meant any 'loader' field on a packed plugin's package.json was dropped. Now each plugin's package.json is read directly once all plugins have been downloaded and moved into place, and the manifest is assembled in a single Node.js invocation. The optional 'loader' field is copied through when present, matching the server-side manifest.json support added in JsPluginManifestEntry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Ran the following commands: ./gradlew docker-server-base:bumpImage docker run --rm ghcr.io/deephaven/server-base:edge pip freeze > docker/server-jetty/src/main/server-jetty/requirements.txt docker run --rm ghcr.io/deephaven/server-base:edge pip freeze > docker/server/src/main/server-netty/requirements.txt
fbff55a to
81d288c
Compare
2 participants
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)