Feature/use native npm

[simschla]

Here is my proposal to get rid of the j2v8 dependency by simply using node.js/npm natively to run the prettier and tsfmt formatters. (We are already using it natively to run the npm install process anyway.)
The node process is spawned and then contacted from within spotless using a REST-api. Let me know what you think...

This should fix #556

[nedtwigg]

Great POC! This passes on my mac. Not sure why it's failing in CI, but you can run the test_npm_8 job on your own machine like so. I didn't look carefully, but it seems like npm install runs every single time, feels slower to start than old one.

[simschla]

Great POC! This passes on my mac. Not sure why it's failing in CI, but you can run the test_npm_8 job on your own machine like so

Also on my mac working. I'll investigate what the problem in ci is exactly.

I didn't look carefully, but it seems like npm install runs every single time, feels slower to start than old one.

For that part I did not actually change anything in the way we do this. NpmInstall is executed in the constructor of the NpmFormatterStepStateBase (or the two subclasses PrettierFormatterStep.State and TsFmtFormatterStep.State respectively).

I was working under the assumption that the state would only be re-created when something in the gradle cache or the configuration was changing and so only then a new npm install would be needed. Is that assumption wrong?

The Node-based rest-server is newly launched every time a formatter function is requested from the steps. But I feel this is the safest way to do it - although this slows down test-runs, because this is done for each test then.

[simschla]

Not sure why it's failing in CI

In a first check the problem seems to be in the target :plugin-maven:npmTest. Running :plugin-gradle:npmTest is working however.

[simschla]

I fixed the ci problems. It appears that the node-server-process spawns child-processes in linux which cannot correctly be terminated via the java ProcessBuilder-API.

-> Current ci problems are upstream IMHO - one of the guava libs fails to resolve. Any ideas what's wrong there?

To my point

I was working under the assumption that the state would only be re-created when something in the gradle cache or the configuration was changing and so only then a new npm install would be needed. Is that assumption wrong?

Is that assumption valid or wrong?

[nedtwigg]

I tried it on a couple projects, worked great on mac & linux. On windows, I have one project that works well, and another that does not. I get:

Unable to apply step 'prettier-format' to 'client\src\main\styles\foo.scss'
com.diffplug.spotless.npm.SimpleRestClient$SimpleRestResponseException: 500: Internal Server Error (Unexpected response status code.)
        at com.diffplug.spotless.npm.SimpleRestClient.postJson(SimpleRestClient.java:70)
        at com.diffplug.spotless.npm.SimpleRestClient.postJson(SimpleRestClient.java:44)
        at com.diffplug.spotless.npm.PrettierRestService.format(PrettierRestService.java:49)
        at com.diffplug.spotless.npm.PrettierFormatterStep$State.lambda$createFormatterFunc$1(PrettierFormatterStep.java:91)
        at com.diffplug.spotless.FormatterFunc.apply(FormatterFunc.java:31)

and once that has happened, subsequent files fail with java.net.SocketTimeoutException: Read timed out. When I kill gradle, the node process lives on and makes it impossible to do a clean until I kill node with task manager.

I added npmTest to the windows CI build, which is passing, which is good. I'm not sure how to debug the 500: Internal Server Error. Presumably the console output of the node process would have something interesting to say, maybe we need a debug flag that dumps that slurps that console output back to the parent Spotless process...

[nedtwigg]

Correction! The problematic behavior is not windows-specific. I have a complex project which works on unix and windows, and a complex project which fails on unix and windows. The only difference is that unix manages to kill the node subprocess, but windows does not. I will try to recreate the 500: Internal Server Error in the test suite.

[nedtwigg]

Okay, the previous commit adds a file which shows the bug. If you run this in the gradle plugin, you get 500 Internal Server Error. If you run the test that I committed, you get that same exception, but the error console also has all of this on stderr, uncaptured in the exception. None of this gets printed to the gradle console by the plugin.

Server running on port 56519
Error: Couldn't resolve parser "postcss"
    at resolveParser (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/prettier/index.js:11343:15)
    at normalize$1 (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/prettier/index.js:11438:18)
    at formatWithCursor (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/prettier/index.js:15034:12)
    at args (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/prettier/index.js:51620:12)
    at Object.format (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/prettier/index.js:51640:12)
    at app.post (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/serve.js:54:40)
    at Layer.handle [as handle_request] (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/express/lib/router/layer.js:95:5)
    at next (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/private/var/folders/7w/jcsr3q7106gbchv8gkvz09w40000gp/T/junit3192173313758712584/build-dir/spotless-node-modules-prettier-format/node_modules/express/lib/router/layer.js:95:5)
graceful shutdown finished.

So it's clearly operator error - I was using the postcss parser in prettier 1.x, which is gone in 2.x (or at least renamed). But we need some way to pipe these errors to the user, and ideally to gracefully recover so that the node process doesn't stay alive forever doing SocketTimeoutException.

[simschla]

So it's clearly operator error - I was using the postcss parser in prettier 1.x, which is gone in 2.x (or at least renamed). But we need some way to pipe these errors to the user.

This should now be correctly ending up in the gradle output after commit 2bc0464

and ideally to gracefully recover so that the node process doesn't stay alive forever doing SocketTimeoutException.

I’m unclear why this happens. The node process should be terminated using the FormatterFunc.Closeable approach. Is it possible that the closing part is not always called?

[nedtwigg]

Great, I just tested and it's no longer leaving ghost processes around on Windows. The error messages make it easy to debug, and it's so great to have access to all the prettier community plugins! I made a few small changes to the changelog and readme.

I think this is ready to merge, I'll let you press the merge button if you agree. Accepting the committer invite I just sent you involves no duties, only privileges. Mostly it means you can restart CI jobs if you think an issue is just flakiness. Happy to press the merge button for you if you'd rather not accept the invite, no worries either way :)

[simschla]

This is great. I’m happy to accept the invite and push the merge button myself 👍 thanks for your (again) great support in implementing this change. 🙏