Adding npm-based formatters typescript-formatter (tsfmt) and prettier #283

simschla · 2018-08-18T06:19:56Z

The goal of this PR is to add two new formatter steps to spotless

prettier.io
tsfmt

This enables spotless to format typescript and all the languages supported by prettier.

The state of the code is currently in draft. A lot needs to be done, but I start the PR now, to be able to discuss on the code rather than in issue #119

jbduncan · 2018-08-18T10:11:09Z

Cool, thanks for submitting this initial PR @simschla!

I can't speak for @nedtwigg, but if you let us know when you feel the code is more or less stable or cleaned-up, then I'd personally be more than happy to chip in my thoughts. :)

nedtwigg

The structure of this is great! You're definitely heading in the right direction.

I just pushed up Formatter is now AutoCloseable, so that facilitates FormatterFunc.Closeable works. #284 which makes FormatterFunc.Closeable actually work.
I would probably change package nodebased to just npm. I think that's the brand that user's will know best.
I'd probably move your subpackages into the root package as well. I think their names are distinct enough to make it clear.
This will allow us to make more of the implementation package-private. The less API we have to maintain the better.

All of the above is nitpicks though, feel free to leave as-is. My only substantive feedback is about the PrettierOptions and TsOptions classes. They look laborious and error-prone to create and maintain, and in the end they just assign values to a map. How about if the state just has TreeMap<String, Object>, and when you create the FormatterFunc you just iterate over the entries? That way users can use all the groovy / kotlin map literals they're used to, and we can delegate to tsfmt and prettier for the documentation.

nedtwigg · 2018-08-20T05:13:33Z

lib/src/main/java/com/diffplug/spotless/JarState.java

+				.map(File::getName)
+				.collect(Collectors.toList());
+	}
+
 	URL[] jarUrls() {


This added method has no usages.

Yeah, that's true for now. I had added the method to try using a custom-compiled j2v8 version. The now used v.4.6 is 2 years old and incorporates a rather old npm version. However, if we would go down the road with a custom built j2v8 version, we would still need to discuss on how to have those os-specific jar files available on client-machines... Any thoughts on this part? Are we fine with a 2 year old j2v8 version or do we need to something about that?

I'm fine with a 2-year old j2v8 (Java 8 is already 4+ years old). If you want to publish your own j2v8 binaries under the spotless namespace, I'd be willing to help you, but I think it'd be better to help j2v8 itself get them published if a 2-year old j2v8 can't get it done.

For now the 2-year-old version is working with prettier and typescript-formatter, so there's no need to act right now. If we intend to upgrade those versions some day or even let the user decide which version to use, we might night to get back to this topic. For now, I'll remove the change.

nedtwigg · 2018-08-20T05:22:14Z

lib-extra/src/main/java/com/diffplug/spotless/extra/nodebased/wrapper/Reflective.java

+import java.util.Objects;
+import java.util.StringJoiner;
+
+public class Reflective {


Let's make this package-private if we can. The less API surface-area we have to maintain the better. This looks useful, but ideally we can accomplish this PR with the only new API being the steps, and keep as much of the implementation private as we can. Might be nice to move this utility into lib as API and improve our other reflection code with it someday...

Agreed, regarding moving this to lib! However, if we go down that route, we should seriously consider mimicking the API of JOOR, which AFAICT has a more user-friendly API than the current one that Reflective exposes.

Heck, we might even consider shading JOOR into lib instead. However, I personally have no experience with incorporating shaded libraries into a Java project, so the only advice I could give would be to look into a Gradle plugin like https://github.com/johnrengelman/shadow and see where that leads.

That being said, my comment above is out-of-scope for this PR and something to consider only when we're ready to move this class in the future. :)

I've just raised a new issue for this: #301

nedtwigg · 2018-08-20T05:27:39Z

...ra/src/main/java/com/diffplug/spotless/extra/nodebased/prettier/options/PrettierOptions.java

+		Optional.ofNullable(getRequirePragma()).ifPresent(val -> v8Object.add("requirePragma", val));
+		Optional.ofNullable(getInsertPragma()).ifPresent(val -> v8Object.add("insertPragma", val));
+		Optional.ofNullable(getProseWrap()).ifPresent(val -> v8Object.add("proseWrap", val));
+		return v8Object;


I see now that all the fields of this object are boxed instead of primitive so that you can use nullability to optionally set fields. We should comment the file to indicate that:

All fields are boxed (e.g. Integer instead of int) to allow null values. A null value indicates that the default value will be used.

nedtwigg · 2018-08-20T22:25:15Z

#284 has been merged, so I merged master into your branch so that the close() method will get run.

simschla · 2018-08-21T05:56:06Z

My only substantive feedback is about the PrettierOptions and TsOptions classes. They look laborious and error-prone to create and maintain, and in the end they just assign values to a map. How about if the state just has TreeMap<String, Object>, and when you create the FormatterFunc you just iterate over the entries? That way users can use all the groovy / kotlin map literals they're used to, and we can delegate to tsfmt and prettier for the documentation.

I see the advantage and elegance of that approach, however, it completely exposes the implementation internals to the user.
Right now, using this approach, we have full control over which config options in what type we pass to prettier/tsfmt. Example: I currently prevent the user to set any of the dryRun?: Boolean, replace: Boolean or verify: Boolean config options towards tsfmt as they would change the behavior of the formatter to no longer be spotless-conform.

What are your opinions on that aspect?

nedtwigg · 2018-08-21T06:07:52Z

we have full control over which config options

I read that as "we have full responsibility for". Spotless integrates with a ton of different formatters. The only way that we're able to keep up is by delegating raw access to the formatters as much as possible. Our main value-add is playing "switchboard", and the less code we write to accomplish that goal the better.

they would change the behavior of the formatter to no longer be spotless-conform

Giving a user power inevitably means giving them the opportunity to shoot themselves in the foot. But in this case, it looks like the number of footguns is relatively small. One way to make this just as safe, but with less responsibility is something like this:

for (String forbiddenOption : Arrays.asList("dryRun", "replace", "verify")) {
  if (options.contains(forbiddenOption)) {
    throw new IllegalArgumentException("Spotless TsFmt does not work if you set '" + forbiddenOption "');
  }
}

simschla · 2018-08-21T07:11:00Z

I like the black-list approach for the options. Will try to implement this today/tomorrow. 👍

I have so far applied some of the proposed changes: renaming and moving to npm package, reducing visibility.

Right now I have issues running the test-suite locally, seems like the TestProvisioner is using a cache folder, that is actually empty. Is there a task I can run to cleanup/re-init the cache used by the provisioner? Or a folder I can delete myself?

nedtwigg · 2018-08-21T07:14:57Z

I think deleting testlib/build/tmp will do it:

spotless/testlib/src/main/java/com/diffplug/spotless/TestProvisioner.java

Lines 74 to 76 in 245b35c

    
           File spotlessDir = new File(StandardSystemProperty.USER_DIR.value()).getParentFile(); 
        
           File testlib = new File(spotlessDir, "testlib"); 
        
           File cacheFile = new File(testlib, "build/tmp/testprovisioner." + name + ".cache");

simschla · 2018-08-21T07:43:52Z

that helped, thanks for pointing it out!

as discussed in PR diffplug#283

nedtwigg · 2018-08-25T17:37:38Z

This PR is looking great to me. I think the next thing to work on is probably the docs in plugin-gradle/README.md. Figuring out how to make the documentation clear will help a lot to figure out what the requirements really are for the step.

Once all of the /Users/simschla are gone, I'll work on getting the @NpmTest running in CI.

simschla · 2018-08-28T06:17:07Z

@nedtwigg I think I now have successfully removed all /Users/simschla remains.
I implemented an NpmExecutableResolver that tries to find the npm binary from a system property (maybe use this for ci?) or from commonly used environment properties.

The Step-Implementation is now pretty far, at least from my perspective. Next up would be integrating the steps into gradle-/maven-plugin and - as you pointed out - updating the documentation.

simschla · 2018-08-29T04:47:51Z

I'm currently working on the Gradle-Integration. For prettier() we said to add it to FormatExtension so that it could be used from various format steps.
Prettier needs to know on what kind of files it operates, it needs either an option [parser](url
https://prettier.io/docs/en/options.html#parser) (explicitly select the parser to use) or [filePath](url
https://prettier.io/docs/en/options.html#filepath) (let prettier infer the parser based on the filename).
I see two ways to achieve this: either delegate all of this to the user (needs to specify one of these options him-/herself) or try to resolve at least one of the files in FormatExtension.target and use this as filepath automatically, if nothing is provided in the configuration. Opinions?

nedtwigg · 2018-08-29T05:26:11Z

Explicit control is definitely the easiest to build. Regardless of whether some users want an implicit option, they could definitely make do with explicit control at first. If we build explicit control first, it doesn't stop us from adding implicit detection later. So I would say that our target for v1 should be the easiest thing, which luckily also happens to be the best thing for some users anyway.

nedtwigg · 2018-09-04T07:53:58Z

This is getting close! I added a (failing) integration test and docs. Here's how I would go about it:

write some docs
add an integration test to match those docs
if the step breaks, fix it

At present, it seems that the step is not as simple as it thinks it is. I had to fix some NPE's to get the extension test to run at all, and it still seems to not respond to input parameters as users would expect. Very close though!

simschla · 2018-09-04T07:57:59Z

Thanks for your help on this. I'm actually in the middle of writing the docs and (while doing that) discovering the NPEs that you probably hit in the integration test. will look into it!

nedtwigg · 2018-09-04T08:03:34Z

Roger, if I caused conflicts feel free to force-push over them - my intent is just to show examples :)

simschla · 2018-09-06T19:10:03Z

I have now commited a first draft for the documentation (and have adapted the implementation so that it fits the documentation 😉)

further, I've taken your integration test and adapted/extended it to check tsfmt and prettier.

I currently don't understand why the ci build is failing - I hoped to fix it by merging the current master, but that did not help... any ideas?

simschla · 2018-09-06T19:10:44Z

(and please feel free to provide feedback on the documentation part or directly correct my english mistakes 🙈)

prettier.io tsfmt

- a test dependency in testlib on lib-extra causes a cyclic dependency failure in eclipse - the only reason why lib's tests are in testlib is to fix a similar cyclic concern: lib tests -> testlib testlib -> lib so our fix was to just put lib's tests into testlib's tests. That workaround is not necessary for lib-extra.

+ spotlessApply

simschla · 2018-09-12T11:42:13Z

don't forget to update lib / lib-extra in the readme with all the checkboxes ;)

Thanks for the reminder, has been adapted.

so if you all would be happy to wait <= 12 hours, then I'd be happy to give this a proper review later today.

Sure thing!

Running on windows seems ok so far, however, I cannot get the ingration test for prettier to run on windows. Receiving the error:

com.diffplug.spotless.npm.Reflective$ReflectiveException: java.lang.reflect.InvocationTargetException
	at com.diffplug.spotless.npm.Reflective.invokeMethod(Reflective.java:79)
	at com.diffplug.spotless.npm.ReflectiveObjectWrapper.invoke(ReflectiveObjectWrapper.java:46)
	at com.diffplug.spotless.npm.NodeJSWrapper.require(NodeJSWrapper.java:45)
[...]
Caused by: net.js:135: Error: EINVAL: invalid argument, uv_pipe_open
    this._handle.open(options.fd);
                 ^
Error: EINVAL: invalid argument, uv_pipe_open
    at Error (native)
    at new Socket (net.js:135:18)
    at createWritableStdioStream (node.js:577:18)
    at process.stdout (node.js:603:16)

(looks a lot like similar issues found on the web)

-> it must have something to do with piping stdin/out to/from gradle(runner) but I cannot really grasp what the problem is. Maybe someone else has any idea?
(The PrettierFormatterStepTest itself is running fine, so it is not an inherent problem to an incompatibility between prettier and windows)

thc202 · 2018-09-13T00:46:37Z

Didn't mean to approve, was still checking :/

lib/src/main/java/com/diffplug/spotless/npm/PlatformInfo.java

lib/src/main/java/com/diffplug/spotless/npm/Reflective.java

plugin-gradle/README.md

lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java

thc202 · 2018-09-13T02:19:03Z

Worth mentioning in the README that the new formats make use of j2v8? I didn't expect that Spotless (indirectly) would create files in my home dir (e.g. libj2v8_linux_x86_64.so).

jbduncan · 2018-09-13T07:10:10Z

I wasn't able to review much of your code in the end due to limited time on my part, so sorry about that! But from what I can tell, structurally things look rather good, and although I did find three nits (see comments below), they're not important and can be left if you feel burned-out.

Overall, I think this is a very, very good PR, especially for delivering such a complex and potentially very useful feature, so well done! 👏

lib/src/main/java/com/diffplug/spotless/JarState.java

simschla · 2018-09-13T15:01:33Z

Worth mentioning in the README that the new formats make use of j2v8? I didn't expect that Spotless (indirectly) would create files in my home dir (e.g. libj2v8_linux_x86_64.so).

Yeah, maybe we can add that to the sections where we remark that nodejs is required?

thc202 · 2018-09-13T15:05:40Z

Sounds good.

simschla · 2018-09-13T15:12:52Z

unless travis picks something up, the PR seems pretty ready from my end. Thanks to all reviewers!

plugin-gradle/README.md

thc202 · 2018-09-13T20:06:09Z

LGTM, thank you!

…-in-j2v8 # Conflicts: # plugin-gradle/README.md # plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java

nedtwigg · 2018-09-18T14:52:49Z

Sorry for the delay, I picked up a flu this weekend which has set off a cascade of schedule failures for me. I'll get to this ASAP.

nedtwigg · 2018-09-24T04:59:50Z

Sorry for long delay, this has been shipped in 3.15.0!

jbduncan · 2018-09-24T08:30:50Z

Fantastic! Well done for all your work on this @simschla. 🎉🎊

source-knights · 2020-05-27T12:11:44Z

@simschla @nedtwigg

Worth mentioning in the README that the new formats make use of j2v8? I didn't expect that Spotless (indirectly) would create files in my home dir (e.g. libj2v8_linux_x86_64.so).

Yeah, maybe we can add that to the sections where we remark that nodejs is required?

Can anyone tell me if it can be changed to not create the libj2v8 file in the home folder but somewhere else? This is disturbing our teams and I fear they might dismiss the whole tool if we cannot fix this.

Also @nedtwigg I think this old discussion above about J2V8 between you and @simschla is related to what I said in #555 This limits us to prettier.io version <= v1.19.0

Thanks and kind regards

nedtwigg · 2020-05-27T18:18:36Z

Re: location of libj2v8 in home folder, I think you'll have to dig around J2V8 for an answer.

Re: prettier.io <= 1.19.0, you are correct, and we're discussing solutions in #556.

source-knights · 2020-05-28T08:13:51Z

Actually it is a bit hard to change the libj2v8 location, because the configuration in that old version of J2V8 is different from what it is doing these days (and no release tag/branch in there), also Spotless does not use a way to call J2V8 with setting that directory but use the build in defaults, so I will add my findings here to make it easier for others:

So the workaround is:

create and add to (or choose a directory that is already in) environment variable PATH (Windows) or LD_LIBRARY_PATH (Linux/Unix)
in Windows delete the prefix "lib" in the filename of the DLL
move the file to the chosen directory
if you changed the environment variables you might need to restart your IDE or shells
the file should not be created anymore in the user home folder

nedtwigg · 2020-05-30T07:35:56Z

@source-knights see #586 for a proposed solution

nedtwigg mentioned this pull request Aug 20, 2018

Formatter is now AutoCloseable, so that facilitates FormatterFunc.Closeable works. #284

Merged

1 task

nedtwigg reviewed Aug 20, 2018

View reviewed changes

simschla added a commit to simschla/spotless that referenced this pull request Aug 22, 2018

using map as config api for npm formatter steps

fe636e8

as discussed in PR diffplug#283

simschla force-pushed the feature/typescript-formatting-using-nodejs-in-j2v8 branch 2 times, most recently from 53e5ba4 to 07a3b5c Compare August 26, 2018 22:38

simschla force-pushed the feature/typescript-formatting-using-nodejs-in-j2v8 branch from 3d8e3e0 to 7b96736 Compare September 6, 2018 18:54

simschla and others added 8 commits September 7, 2018 08:43

initial implementation of nodebased steps

75ce9aa

prettier.io tsfmt

Separate the NpmTest tests from the rest of the test suite.

c0bb96f

renaming package nodebased -> npm

991457c

move subpackages to npm top-level package

164f047

+ spotlessApply

restrict visibility

37fff43

revert the from-file feature for JarState as it is not used right now

ddfc882

adapt to package renaming

ef4d244

simschla force-pushed the feature/typescript-formatting-using-nodejs-in-j2v8 branch from 6ee4e77 to a662f48 Compare September 12, 2018 09:04

fixes for running on windows

ade884a

simschla force-pushed the feature/typescript-formatting-using-nodejs-in-j2v8 branch from a662f48 to ade884a Compare September 12, 2018 09:22

thc202 approved these changes Sep 13, 2018

View reviewed changes

thc202 reviewed Sep 13, 2018

View reviewed changes

lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java Show resolved Hide resolved

jbduncan approved these changes Sep 13, 2018

View reviewed changes

lib/src/main/java/com/diffplug/spotless/JarState.java Outdated Show resolved Hide resolved

thc202 reviewed Sep 13, 2018

View reviewed changes

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

simschla force-pushed the feature/typescript-formatting-using-nodejs-in-j2v8 branch from 4dc8831 to c341a9e Compare September 13, 2018 17:26

integrating PR feedback

807bf7b

simschla force-pushed the feature/typescript-formatting-using-nodejs-in-j2v8 branch from c341a9e to 807bf7b Compare September 13, 2018 18:16

Merge branch 'master' into feature/typescript-formatting-using-nodejs…

60f2bcd

…-in-j2v8 # Conflicts: # plugin-gradle/README.md # plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java

nedtwigg merged commit 8506a63 into diffplug:master Sep 24, 2018

fvgh mentioned this pull request Oct 25, 2018

Add prettier to Maven/Gradle CSS extensions #312

Closed

simschla deleted the feature/typescript-formatting-using-nodejs-in-j2v8 branch June 2, 2020 14:58

Adding npm-based formatters typescript-formatter (tsfmt) and prettier #283

Adding npm-based formatters typescript-formatter (tsfmt) and prettier #283

Conversation

simschla commented Aug 18, 2018

jbduncan commented Aug 18, 2018

nedtwigg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedtwigg commented Aug 20, 2018

simschla commented Aug 21, 2018

nedtwigg commented Aug 21, 2018

simschla commented Aug 21, 2018

nedtwigg commented Aug 21, 2018

simschla commented Aug 21, 2018

nedtwigg commented Aug 25, 2018

simschla commented Aug 28, 2018

simschla commented Aug 29, 2018

nedtwigg commented Aug 29, 2018

nedtwigg commented Sep 4, 2018

simschla commented Sep 4, 2018

nedtwigg commented Sep 4, 2018

simschla commented Sep 6, 2018

simschla commented Sep 6, 2018

simschla commented Sep 12, 2018

thc202 commented Sep 13, 2018

thc202 commented Sep 13, 2018

jbduncan commented Sep 13, 2018

simschla commented Sep 13, 2018

thc202 commented Sep 13, 2018

simschla commented Sep 13, 2018

thc202 commented Sep 13, 2018

nedtwigg commented Sep 18, 2018

nedtwigg commented Sep 24, 2018

jbduncan commented Sep 24, 2018

source-knights commented May 27, 2020 • edited

nedtwigg commented May 27, 2020

source-knights commented May 28, 2020

nedtwigg commented May 30, 2020

source-knights commented May 27, 2020 •

edited