tsfmt maven plugin

[source-knights]

As discussed in #552 creating a new PR for this

[nedtwigg]

I fixed the warning about -color, no change in behavior. The tsconfig.json is still failing with No inputs were found . I think it's fine to just remove tsconfig.json support from the maven plugin, although I would like to get to the bottom of it because I suspect we'll learn something important.

Another important point is that the inline configuration does not appear to be working. It is indenting with 4 spaces rather than 1, whereas we are getting the correct result when reading the same config directly from tsfmt.json. I wonder if there's a datatype issue in the XML parsing (e.g. boolean vs int vs string).

[nedtwigg]

Yep, the inline config was broken due to datatypes problem, as the previous commit shows. But now it's fixed, so we're almost done!

The tsconfig test is still broken. I don't want to advertise in the docs that it works, when the tests show that it doesn't. Here's the two things I need from you before I can merge and release:

• figure out why tsconfig is broken and fix OR remove tsconfig from maven docs and impl
• fix the buildDir default (the annotation must do what it says, but I don't care what it says)

[nedtwigg]

This is what I mean by "buildDir" default. It appears to not actually be readonly, and it also appears that the default does not work. One solution is for the "buildDir" to be just a regular nullable parameter, since it seems that's how you're actually using it.

[source-knights]

This was put in due to the tsftmt step formatter wants to have the build dir. In the Gradle impl that seems to be easier, but here in the Maven this seems to be the way. At least that Parameter(defaultValue = "${project.build.directory}" is used somewhere else (and seems to work with all the tests, except maybe the tsconfig test).

I still think the error message with tsconfig is missleading, cause when I looked at it, it was actually mentioning the found file name of the test.ts, so it does not really seem to be an error with missing inputs.

No time to look at this right now, maybe tomorrow. Thx @nedtwigg

[nedtwigg]

Sounds good. My assertion is that defaultValue has no effect, because later on you say "if buildDir is null then set it to some default value". That logic, of setting a default value if it is null, is fine. My objection, perhaps mistaken, is that the parameter annotation is claiming to magically set a default value, but it isn't working. So if it isn't working, it shouldn't be there.

If there are other instances like this in the codebase, then I think they are broken too.

[nedtwigg]

Also, I'm skeptical that new File(".") will work well as a default value for multiproject setups, but I'm willing to merge and ship this and wait for someone to have a problem and then we can worry about fixing it.

[source-knights]

I think you will get null if you start this without a maven/gradle setup (e.g. in a unit test straight in IDE). Therefore I set the ".". But might be wrong

[nedtwigg]

I tried deleting it, and I saw failures in the maven integration tests, which run using "real maven".

[source-knights]

Just fixed the buildDir, it is now getting the correct dir when I debugged through it. Seems that the ${project.build.directory} is only evaluated or returning correct value in that Mojo class.

[source-knights]

@nedtwigg Tsconfig test is working now, but only when I use "files" with a direct link to "src/main/typescript/test.ts" in the tsconfig file.

If I leave "files" out and just use

"include": [
	 "src/main/typescript/*.ts"
	 	]

it does fail with

C:\Users\xyz\AppData\Local\Temp\junit3377288995823290344\target\spotless-node-modules-tsfmt-format\node_modules\typescript-formatter\lib\utils.js:64:
Error: No inputs were found in config file 'tsconfig.json'. Specified
'include' paths were '["src/main/typescript/*.ts"]' and 'exclude' paths
were '["nothing.xml"]'. -> [Help 1]

[source-knights]

I found the real problem. The fact that the FileLocator class copies the settings/config files in the target directory under a temporary name leads to this problem with tsconfig file. Seems the tsfmt implementation then looks in the wrong paths when using "include"/"exclude" while using the correct one with "files" in the json config. Instead of using that FileLocator impl switching to a straight file location works (which is actually what the gradle impl does too, comparing the actually used paths during debugging).

@nedtwigg this is solved now as far as I can see

[nedtwigg]

Thanks very much for getting to the bottom of this! I now understand your fix better, and understand why my commits broke it, but I'm too sleepy to finish this right now.

I did not realize until now that every config file was being copied to a temp location. I tried to rip that out, but it fails because we need maven's resource manager to resolve the "${basedir}" stuff, and the API maven gives us to parse those refuses to say where the files actually are on disk.

Since we understand the limitation, my plan for tomorrow is to

1. remove tsconfig support
2. merge and release this PR in a new version
3. make a new issue which says "Why does the maven plugin copy every config file to a random temp location? It is breaking the typescript usecase".

Is this okay with you? If a maven wizard comes along with a fix, then we get a perf improvement for the whole plugin, and we get an elegant implementation for the tsconfig.json case that we've struggled with. And if we don't get a wizard, we've documented this problem for others which will have it, rather than sweeping it under the rug into a weird one-off implementation for this one step.

[source-knights]

@nedtwigg Not sure if you need to change anything after my last commit for the tsconfig. The FileLocator puts stuff in target/build folder and creates a temporary name instead of the e.g. "tsconfig.json". As I am skipping this now the ${project.basedir} in the maven xml is still used, the file is just not copied as a temp filename then. Work fine for me.

So in short, the new solution will still replace the base ${project.basedir} property.

[source-knights]

It's just that ${project.basedir}/tsconfig.json will not be processed to c:\Users\xyz\AppData\Local\Temp\junit12342425646563626/target/spotless_temp_8475628489474754.json but stay c:\Users\xyz\AppData\Local\Temp\junit12342425646563626/tsconfig.json

To proof that the property is replaced and not just uses current dir, you can even put something like "TEST/${project.basedir}/tsconfig.json" in the maven xml and it will be replaced to "TEST/c:\Users\xyz\AppData\Local\Temp\junit12342425646563626/tsconfig.json" and not found then of course.

[nedtwigg]

A-haaaaaa. So the ${basedir} is just string replacement that happens in the XML to POJO mapping, it's not our responsibility to do it. Well then what the heck is our resource manager for!? Lol. That's a question for another PR. Thanks very much. Sorry for slow response.

[nedtwigg]

Released in 1.29.0