Add support for typescript-formatter

[simschla]

I'm building a custom spotless rule for formatting typescript files using typescript-formatter. This all works well at the moment, however, my custom rule takes rather long, since I need to spawn a system process (using gradle "exec") for every single file. typescript-formatter, however, would have the ability to format a list of files all at once which would tremendously speed up things. It would be nice to have spotless support to do this "all files at once".

my current spotless format rule:

File nodeDistBin = (File) fileTree("${rootProject.buildDir}/nodejs").include("**/node").find()

...

    format 'typescript', {
        target fileTree(project.projectDir).include('**/*.ts').minus(EXCLUDED_DIRS)
        bumpThisNumberIfACustomStepChanges 1
        custom 'tsfmt', { String input ->
            String result = ""
            File.createTempFile("typescript-formatter", ".ts").with { File file ->
                deleteOnExit()
                file.write(input, 'UTF-8')
                exec {
                    workingDir "$rootProject.projectDir"
                    executable "${nodeDistBin.absolutePath}"
                    args "node_modules/typescript-formatter/bin/tsfmt", "--replace", "--useTslint", "tslint.json", "${file.absolutePath}"
                }
                result = file.getText('UTF-8')
            }
            return result
        }
        indentWithSpaces(4)
        trimTrailingWhitespace()
        endWithNewline()
    }

[nedtwigg]

Isn't that incremental up-to-date checking handy though ;-)

Even if we batched these calls, you'd still be doing filesystem IO per file. Compare that to the three steps below your custom step indentWithSpaces, trimTrailingWhitespace, and endWithNewline - each step can be composed on top of the previous without any filesystem IO.

Spotless requires each formatter step to be a function that turns a string into a string. This allows Spotless to protect the user from all kinds of misbehaving formatters, and to efficiently compose multiple formatters. Changing this model to support batching would be very difficult, and it couldn't help with the filesystem IO part of your performance problem.

The right way to do this would be to create a new FormatterStep that calls into the node.js as a lib using j2v8. If you get this working in your buildSrc directory, we'd love to make it a part of spotless core, I'm sure you're not the only one looking for typescript support.

[simschla]

@nedtwigg Meanwhile I have a solution for batchprocessing typescript files on initial spotless check, while picking results from previously batch-processed typescript files when actual spotless call is performed.

I would be willing to give the solution using j2v8 a try on my end. It would probably be best to create a new formatter step for this. correct?

In order to be able to use NodeJS embedded in j2v8, I still have to manage a node_modules folder for keeping all dependencies and the modules tsfmt or prettier (as typescript formatting options) themselves. How should I solve that in a generic way?

1. Creating the node_modules during build (even when caching) requires a node installation on the machine running spotless. (not an option in my opinion)
2. Somehow figuring out how to use j2v8 itself for creating the node_modules. (probably a dead-end telling from what little I could find on SO or Google on the matter).
3. Creating the node_modules on my end and reusing them on spotless run would need a prepared node_modules zip file (added to lib-extra?). If the dependencies contain native dependencies that zip would even be needed to be created for all supported platforms. (3b would be to use something like pac

Suggestions? Ideas?

[nedtwigg]

A fourth option:

• shell out to npm to create the node_modules, then use j2v8 to run them

public class NpmFormatterStepBuilder {
  List<String> npmDeps;
  String scriptToExecute;

  static class State implements Serializable {
    String scriptToExecute;
    FileSignature nodeModulesContent;
    transient File nodeModulesDir;
    transient j2v8 j2v8Instance;
  }

  State createState() {
    <make random folder in build dir>
    <shell out to npm to use that folder as node_modules, and download its deps there>
    <use FileSignature to verify that the node_modules doesn't change>
    <create a j2v8 that uses the random folder as its node_modules>
  }

  static String format(State state, String input) {
    <use j2v8 to run state.scriptToExecute against the state.nodeModulesDir>
    <maybe script takes argument spotless_input and returns spotless_output?>
  }  
}

The two main problems with the structure above are:

• calculating State is expensive, so up-to-date checking will be slow. I can help with that, we have existing hacks in place for optimizations like this
• j2v8 needs to be closed. We also have an existing hack for lifetime management like this.

If something similar to the structure above was working, it would be relatively easy to solve the two performance / resource problems mentioned above.

[simschla]

shell out to npm to create the node_modules, then use j2v8 to run them

That was actually what I meant with my first option ;) - it requires to have a matching npm/node installation on the machine running the formatting.

I see a slightly easier way using the gradle wrapper and https://github.com/srs/gradle-node-plugin - this installs a local node/npm on the fly which I can use to create the node_modules without requiring the user to install anything on the system itself.

[nedtwigg]

That was actually what I meant with my first option

Apologies, you stated that very clearly :)

installs a local node/npm on the fly which I can use to create the node_modules without requiring the user to install anything on the system itself

Gradle requires a JDK on the machine. Most folks doing typescript or really any frontend dev are going to have npm on their machine. gradle-node-plugin is fantastic, but the auto-download feature has been a pain in the neck for me on CI servers compared to just using a docker image that already has npm installed. I think us JVM heads want gradle to do all-the-things (reasonable), but people who work primarily in other fields (e.g. frontend) prefer a more conventional approach (also reasonable).

If you build the typescript thing to say "pass me the npm executable to use", then you have options: pass it explicitly, procure it using gradle-node-plugin, or grab it from the path. I think adding a hard dep on gradle-node-plugin puts you in a corner, especially if you can shrink your surface area pretty easily to just "pass me npm".

Spotless has 4 artifacts: lib (no deps), lib-extra (a few deps, no build system deps), plugin-gradle, and plugin-maven. It's 100% fine to add something that only works in one build system, but ideally users of other build systems can at least have a chance to submit PR's to integrate it into their build system of choice.

[simschla]

Yeah, I see where you're going - makes sense to keep the lib/lib-extra with as few dependencies as possible.

Let's try it this way and see how it is going.

On the progress side:

given there is a prepared node_modules on the system to use (however that happened :)),
I have a running proof-of-concept that uses

• prettier.io to format typescript files using j2v8 (would also allow options to format other file-types (such as javascript, json, css, scss, yaml, markdown)
• typescript-formatter to format typescript files using j2v8 (can only handle typescript, but is faster at it)

Measuring the performance of this proof-of-concept on my macbook pro:

Engine.	Repetitions	Lines of Code	Avg. formatting time	Desc
prettier.io	1000x	659	123.817 ms	Reusing J2V8 NodeJS runtime for the runs
prettier.io	100x	659	1100.48 ms	Fresh instance of J2V8 NodeJS runtime for every run
prettier.io	1000x	22	5.240 ms	Reusing J2V8 NodeJS runtime for the runs
prettier.io	100x	22	721.43 ms	Fresh instance of J2V8 NodeJS runtime for every run
tsfmt	1000x	659	27.701 ms	Reusing J2V8 NodeJS runtime for the runs
tsfmt	100x	659	607.27 ms	Fresh instance of J2V8 NodeJS runtime for every run
tsfmt	1000x	22	2.282 ms	Reusing J2V8 NodeJS runtime for the runs
tsfmt	100x	22	515.87 ms	Fresh instance of J2V8 NodeJS runtime for every run

As a result of this measurement, it seems to be absolutely important, that we can keep the J2V8 engine between the formatting of two files (in the same spotless run). That shouldn't be a problem, should it?

[nedtwigg]

Great! We have a similar problem in that we create a bunch of classloaders that need to be closed:

https://github.com/diffplug/spotless/blob/master/lib/src/main/java/com/diffplug/spotless/SpotlessCache.java

We can change SpotlessCache to be something more generic, maybe put(Serializable key, Runnable onClose)?

Very excited for this improvement - supporting the entire npm ecosystem is a big deal!

[simschla]

@nedtwigg Update on my status and a few questions:

I now have an initial implementation of the two formatter-steps typescript-formatter tsfmt and prettier-formatter prettier (still a draft, will need cleanup).

However, I have a few questions:

1. I see there is a com.diffplug.spotless.FormatterFunc.Closeable interface, intended to be used when a formatter function needs to cleanup resources once it is no longer used. This would be ideal in my case of a native-backed formatter! However, I find no usage of the interface in spotless. Is that dead code that should not be used?

2. In the contribution guidelines, you write, a new formatter-step "Has a test class named SomeNewStepTest" - I'm unsure on how to proceed on this in my case, since my steps need a native installation of npm on the host running the test in order to do anything at all. Ideas? Opinions?

3. I think the current configuration concept is based on formatter-types (e.g. java, kotlin, ... and in there then the configuration of the steps themselves).
   The prettier formatter step is a little bit a different beast, as it is able to handle a lot of languages:

• JS-Style
  • JS ES2017
  • JSX
  • Flow
  • Typescript
  • Vue
  • JSON
• CSS-Style
  • CSS3+
  • Less
  • SCSS
  • styled-components
  • styled-jsx
• GraphQL
• GraphQL Schemas
• Markdown-style
  • CommonMark
  • Github Flavored Markdown
  • MDX
• YAML
  Should we create a config extension for all of them separately (and internally use the prettier engine) or should we create a prettier-config-extension where one can select the parser/formatter time within?

Also, feedback on the current (draft) state of code is very welcome :)

[nedtwigg]

1. I see there is a com.diffplug.spotless.FormatterFunc.Closeable interface, ... I find no usage of the interface in spotless. Is that dead code that should not be used?

Ruh-roh. I don't know for sure, but sounds like it! I'll take a deeper look this weekend.

2. a new formatter-step "Has a test class named SomeNewStepTest"

I think we can use JUnit categories to mark these.

/** Marker class for tests that need a native NPM to work. */
public static interface NeedsNpm {}

@Category(NeedsNpm.class)
public class SomeNewStepTest {}

Then we setup gradlew test to exclude NeedsNpm, and gradlew testNpm will run the tests that require NPM. If you add me as a collaborator or open a PR then I can push this up sometime this weekend. The order of priorities:

1. random user clones and runs gradlew test shouldn't rely on npm install
2. ideally CI server would run these tests too, but not the end of the world if not (e.g. it would be great if we did CI on windows, but we don't)
3. we can edit .travis.yml to install an npm for us

3. The prettier formatter step is ... able to handle a lot of languages

My inclination is to integrate prettier into FormatExtension, so that it's available for all languages, a-la LicenseHeaderStep:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Lines 421 to 431 in f622379

	/**
	* @param licenseHeaderFile
	* Content that should be at the top of every file.
	* @param delimiter
	* Spotless will look for a line that starts with this regular expression pattern to know what the "top" is.
	*/
	public LicenseHeaderConfig licenseHeaderFile(Object licenseHeaderFile, String delimiter) {
	LicenseHeaderConfig config = new LicenseFileHeaderConfig(delimiter, licenseHeaderFile);
	addStep(config.createStep());
	return config;
	}

Then it could be used like this:

spotless {
  format 'graphQl', {
    target 'src/main/resources/**/*. graphql'
    prettier()
  }
  freshmark {
    target '*.md'
    prettier()
  }

This also applies to @fvgh's work on integrating Eclipse's WTP formatters. I was thinking the same eclipseWtp() for FormatExtension there, but not sure...

[nedtwigg]

Also, can't wait to review code, won't have time til this weekend. Can you open a PR? I won't merge till we're ready, but I think it's easier to discuss in that format.

[simschla]

Thanks for your feedback. I just created the PR - will get back to adressing your answers after the weekend.

[nedtwigg]

This has been resolved since September 2018 :P Closing Feb 2019. Thanks again @simschla, great contribution!