Feature/use native npm #606

simschla · 2020-06-07T19:33:44Z

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

namely start a node-server and rest-call into that node-server

due to missing package.json files

and improve http resource handling

- I'm a big fan of prettier, and use it a lot myself - But I don't want npm to be a must-have for Spotless devs - That is why we have a separate CircleCI job `test_npm_8`, which is the only one that requires npm

nedtwigg · 2020-06-08T01:18:14Z

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 · 2020-06-08T13:10:02Z

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 · 2020-06-08T13:33:32Z

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.

somehow the mavenRunner got stuck when there was no stdout/stderr output. Fixing implementation as to detect when that is the case and to simplify things actually read stdout/stderr within the actual main thread, not seperately spawned threads.

the problem appears to be that on unix, the node-serve call is spawning sub-processes. Subprocesses, however, are obviously not correctly terminated by the java ProcessBuilder-API. The current solution is to actively call the rest service and request a shutdown of the process from within the api and only fall back to ProcessBuilder api if that does not work.

simschla · 2020-06-09T13:40:49Z

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 · 2020-06-11T19:41:24Z

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 · 2020-06-11T19:50:11Z

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 · 2020-06-11T20:13:48Z

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.

plugin-gradle/README.md

simschla · 2020-06-11T21:22:56Z

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?

…d correctly

nedtwigg · 2020-06-12T22:51:39Z

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 · 2020-06-12T23:20:35Z

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. 🙏

simschla and others added 24 commits June 7, 2020 21:27

poc: call native node-server from within java

cf00d4d

resolve prettier config once only

b07af27

extract http/url handling part into a pretty simple client...

2180d35

raw state: start node server and execute rest calls to it

3c8d2ca

use native node approach for tsfmt

372107a

namely start a node-server and rest-call into that node-server

remove dependency to j2v8

51da6fe

spotlessApply

9999bdd

add more details in case npm autodiscovery fails

03fbd98

remove j2v8 reference from readme

8f88dc2

cleanup server startup code

589940c

extract file handling to helper class

48ce207

refactor and use npmProcess helper instance

3dcb1ec

reduce log noise from npm install call

78a0ea8

due to missing package.json files

only listen on localhost

bc3c86a

cleanup logging

57f97b9

enable spotless for js formatting

d7f3564

format js files using spotless

ee2dd1d

refactoring: extract inner classes and renamings

fbb3961

and improve http resource handling

adapt changelog

eafb850

adapt to newer copyright header

d47a949

try to fix circle-ci (use node for running internal spotlessJavascript)

674044b

Remove javascript from our dogfooding.

c70bc85

- I'm a big fan of prettier, and use it a lot myself - But I don't want npm to be a must-have for Spotless devs - That is why we have a separate CircleCI job `test_npm_8`, which is the only one that requires npm

Fix spotbugs warning.

12680c5

Windows cache seems to be broken, we'll just let it be slow.

92197e1

simschla added 2 commits June 8, 2020 21:02

fixing ci build

c0879e1

somehow the mavenRunner got stuck when there was no stdout/stderr output. Fixing implementation as to detect when that is the case and to simplify things actually read stdout/stderr within the actual main thread, not seperately spawned threads.

nedtwigg added 3 commits June 11, 2020 11:43

Make the windows build run the npmTest

42309c7

Oops, windows build doesn't want './'

f5e7e6c

Oops, we need to store the test results too!

d319266

Add a test that recreates the problem I'm seeing.

5764f4c

simschla added 3 commits June 11, 2020 22:30

bump version numbers to latest

a12343f

improve robustness and error feedback

2bc0464

adding a test to prove that we can support prettier-plugins now

6f8c550

nedtwigg reviewed Jun 11, 2020

View reviewed changes

plugin-gradle/README.md Outdated Show resolved Hide resolved

nedtwigg reviewed Jun 11, 2020

View reviewed changes

plugin-gradle/README.md Outdated Show resolved Hide resolved

simschla force-pushed the feature/use-native-npm branch 2 times, most recently from b11c751 to 89b99a4 Compare June 11, 2020 21:56

documenting the community-plugin benefit and adding more examples

0808fc2

simschla force-pushed the feature/use-native-npm branch from 89b99a4 to 0808fc2 Compare June 11, 2020 22:15

simschla added 2 commits June 12, 2020 07:23

PR feedback: try to avoid **/*.filetype as target examples

e2981e5

PR feedback: add links to prettier plugins

8eff924

simschla force-pushed the feature/use-native-npm branch from 8cbbb27 to 6993d02 Compare June 12, 2020 09:28

simschla added 2 commits June 12, 2020 11:36

add support for prettier-plugins to maven-builds and document in README

dd6f120

adapting test for new output format

bc9410b

simschla force-pushed the feature/use-native-npm branch from 73b6894 to bc9410b Compare June 12, 2020 09:37

simschla and others added 3 commits June 12, 2020 11:49

and verify what we actually learned: error message needs to be relaye…

38053bc

…d correctly

Update changes.

08d8321

Remove some empty newlines.

a69670e

simschla merged commit e7dbfc8 into diffplug:master Jun 12, 2020

simschla deleted the feature/use-native-npm branch January 10, 2023 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/use native npm #606

Feature/use native npm #606

simschla commented Jun 7, 2020 •

edited

Loading

nedtwigg commented Jun 8, 2020

simschla commented Jun 8, 2020

simschla commented Jun 8, 2020

simschla commented Jun 9, 2020

nedtwigg commented Jun 11, 2020

nedtwigg commented Jun 11, 2020 •

edited

Loading

nedtwigg commented Jun 11, 2020

simschla commented Jun 11, 2020

nedtwigg commented Jun 12, 2020

simschla commented Jun 12, 2020

Feature/use native npm #606

Feature/use native npm #606

Conversation

simschla commented Jun 7, 2020 • edited Loading

nedtwigg commented Jun 8, 2020

simschla commented Jun 8, 2020

simschla commented Jun 8, 2020

simschla commented Jun 9, 2020

nedtwigg commented Jun 11, 2020

nedtwigg commented Jun 11, 2020 • edited Loading

nedtwigg commented Jun 11, 2020

simschla commented Jun 11, 2020

nedtwigg commented Jun 12, 2020

simschla commented Jun 12, 2020

simschla commented Jun 7, 2020 •

edited

Loading

nedtwigg commented Jun 11, 2020 •

edited

Loading