Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS Plugins development is slow after #4389 #4817

Open
niloc132 opened this issue Nov 13, 2023 · 8 comments
Open

JS Plugins development is slow after #4389 #4817

niloc132 opened this issue Nov 13, 2023 · 8 comments
Assignees
Labels
bug Something isn't working plug-ins triage
Milestone

Comments

@niloc132
Copy link
Member

Note: this may not affect deephaven.jsPlugins.resourceBase, but while this is documented to exist at https://deephaven.io/core/docs/how-to-guides/configuration/js-plugins/#configuration, no mention is made of "you must have a file named manifest.json nor what the format of that file should be. I'm also not at all sure that it entirely solves the problem, since at least a restart is still required after any trivial change.

Also note that the deephaven.jsPlugins.<pluginname> is not documented at all, either at the above link, or at https://deephaven.io/core/docs/how-to-guides/use-plugins/.


When specifying each plugin to use based on deephaven.jsPlugins.<pluginname>, the entire directory specified will be copied rather than either just copying the important files, or referencing the filesystem location. This causes some irritation when developing js plugins.

First, the entire directory is copied. The specified directory must contain a package.json file. In NPM/etc projects, that directory also contains npm_modules/ dir containing both build-time and runtime dependencies, and can easily be many MB. All sources tend to also be listed in a src/, which is also unnecessary. The only path currently read from this file is the main path - arguably we should instead read the files array property https://docs.npmjs.com/cli/v6/configuring-npm/package-json#files

The optional files field is an array of file patterns that describes the entries to be included when your package is installed as a dependency. File patterns follow a similar syntax to .gitignore, but reversed: including a file, directory, or glob pattern (, **/, and such) will make it so that file is included in the tarball when it's packed. Omitting the field will make it default to ["*"], which means it will include all files.

Respecting files has the downside of further leaning on the package.json format explicitly, rather than just happening to use a few of the same keys.

Second, by copying files instead of referencing their location on disk as we formerly did means that any change made to the JS source code means not only rebuild the output files (which can usually be automatically performed easily with a "live" or "watch" mode for building), but also restarting the dhc server to pick up those changes, copy them to the required zip file. While the first issue can be fixed by manually specifying a manifest file, this second applies to both cases.

@niloc132 niloc132 added bug Something isn't working triage plug-ins labels Nov 13, 2023
@devinrsmith devinrsmith added this to the Backlog milestone Nov 14, 2023
@devinrsmith
Copy link
Member

I can't generally speak about JS development workflows, so I've deferred to others. My high-level take is that I'm "ok making development workflows harder if it makes end-user workflows much easier". I think that the manifest based approach is not really friendly to end-users.

The impetuous behind #4389 was to lay the groundwork for installing js-plugins from python wheels (#2772). I don't know what the status of packaging the js content in python wheels is (cc @jnumainville @mofojed), but it's a really easy deephaven-core uplift to integrate python wheel js content when ready.

We might be able to by-convention exclude npm_modules/ from copying, although it's not a technically "clean" solution, it may be a compromise we are willing to make.

The related documentation tickets can be prioritized if necessary (https://github.com/deephaven/deephaven.io/issues/3272, https://github.com/deephaven/deephaven.io/issues/3271 cc @jjbrosnan @margaretkennedy).

@mofojed
Copy link
Member

mofojed commented Nov 27, 2023

@devinrsmith @jnumainville has the plotly-express plugin packaging js with the python. It also uses npm to only package the package that will get published (ie. stuff that is defined in files in the package.json), so it doesn't include the node_modules.
As for dev workflows vs end-user workflows, in my perfect scenario it's just a pip install deephaven-plugin-x for the end-user, and it's just a pip install -e /path/to/deephaven-plugin-x for developers (#93). I think we're almost there with the end-users. For developer workflow, I want to continue making those improvements as that is important for our own development.

@devinrsmith
Copy link
Member

devinrsmith commented Nov 27, 2023

So, I think we can get a majority of the way there wrt the copying-problem by simply ignoring node_modules and the other always ignored files (.git, etc) - IMO, that's easier than trying to finely parse and grok files.

As far as trying to get back to a state where we can JS files can be edited and picked up on the server by the fly, that is somewhat at odds w/ typical production use cases and I'm wondering if it's an essential part of the JS development experience? My hope would be that a simple DH server restart would be fast enough. (And if copying large files was part of the slowness otherwise experienced, hopefully that can be solved by the earlier point.)

@mofojed
Copy link
Member

mofojed commented Nov 28, 2023

For dev that may be okay but for production we definitely should respect the files defined in package.json - that's the standard for making a JS package. It's easy to do this with npm, why do it a different way?
Maybe we can set up a watcher to restart the server when files have changed, so it's like "hot reloading". Ideally the server wouldn't need to restart and just the browser UI would reload, but I'll take restarting the server as a first step. Asking if it's an essential part of the JS development experience; well that's like asking if autocomplete, syntax highlighting, faster build times, etc. are essential for development experience - all of these tools improve the experience, and we should be using them when possible. Another thing I have not yet addressed is setting breakpoints in JS plugin code, and I do not know how best to address that, but that is something else that would improve the dev experience.

@devinrsmith
Copy link
Member

For production, the files should already be constructed as desired, either delivered via wheels or tar from npm.

@devinrsmith
Copy link
Member

In the case where a developer is pointing at a package.json directory, is watching just the top level directory enough? https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/WatchService.html could be relevant; and I can see a path forward for reloading for development use cases.

@mofojed
Copy link
Member

mofojed commented Nov 28, 2023

For JS we'd probably want to watch the package.json and the dist/index.js (referenced from the package.json main) and that would be sufficient.

@mofojed
Copy link
Member

mofojed commented Nov 29, 2023

For JS may be able to get around this by actually proxying the JS plugins directly to the Web UI: https://github.com/deephaven/deephaven-plugins/pull/140/files
That doesn't necessarily help developers of other plugins though. I haven't tested out this process yet to try it.

devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue Dec 7, 2023
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 added a commit that referenced this issue Dec 14, 2023
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 #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/".

Adds testing, fixes #4893
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working plug-ins triage
Projects
None yet
Development

No branches or pull requests

3 participants