feat: handling globbing internally for Prettier

[wincent]

In this commit we handle globbing internally for Prettier (future commit will do the same for ESLint, but I want to make things at least somewhat readable by splitting this up), which means that we can switch from spawning the prettier executable to calling its API directly internally.

Motivation:

• Frontend checks got turned off in CI because they were global.
• We need to be able to target a subset of files in CI.
• That is difficult because we are relying on how two different tools (ESLint and Prettier) deal with two kinds of globs each ("include" globs passed on the command line and "exclude" globs passed in via .eslintirgnore/.prettierignore files).
• ESLint is using one glob engine and supporting utilities (minimatch, is-glob, and glob-parent), and these in turn depend on is-extglob, brace-expansion, and balanced-match.
• Prettier has its own glob engine.
• So we have multiple kinds of globs, multiple semantic differences, implementation differences and edge cases, and we were trying to navigate those by following a "lowest common denominator" approach.
• Additionally, we had the historical constraint that liferay-npm-scripts should accept a single set of globs for formatting and linting (and csf did the same, for all filetypes), and another self-imposed restriction that we wanted to apply different rules for selecting files to lint (ie. "modern" JS) and files to format (ie. all JS).
• Finally, both tools have major quirks about empty file lists -- that is, if the combination of all the above criteria ends up being that in a particular project everything gets ignored, then they will hard-fail -- requiring us to apply awkward workarounds (like listing both *.es.js and *.js in our globs even though that looks redundant).

For more, see: https://issues.liferay.com/browse/LPS-97644

So, with that context out the way, this commit:

• Isolates us from differences between the tools' globbing behaviors.
• Switches us to programmatic invocation (which will allow us to bypass command line argument length limits).
• Enables us to produce quieter output in CI.
• Is as fast or faster than the previous implementation due to shortcircuiting.
• Provides a model for how we could use Prettier inside csf in the future (which would enable us to format JS inside .jsp files).
• Enables us to be more flexible with whitespace in our ".prettierignore" file, making it similar to "portal-impl/src/portal.properties" in liferay-portal (a request Brian made).

Obviously this is a big change and a bit of a work in progress, so expect it to evolve in future commits, but I have tested it in liferay-portal with this diff:

https://gist.github.com/wincent/7fcc71cda45cdad7098692b38388d9a3

[wincent]

I think there'll be one failure in the tests caused by me trying to read .prettierignore when there isn't one there. Otherwise it should be green.

[wincent]

I'll merge this and continue iterating. I'll push follow-ups for any feedback that comes in.

🤠

[bryceosterhaus]

Preface: I understand the changes and the reasoning behind it, and am not actually opposed to the changes. Just wanted to add some thoughts.

I understand the need for wanting to use the same globbing engine for both eslint and prettier, but part of these changes worry me that we are adding complexity to the system that we originally intended to get away from. One reason it was nice to rely on the executables is that it is logic we don't have to maintain. It seems like as we continue down this road that we will end up just writing csf over again. I just don't want to keep going down this road of re-writing logic that might already exist in a tool just to fit a specific side issue we might be facing.

[wincent]

Yep, I hear you and agree with you. Which is why this isn't the first path that I went down. But when we got told we couldn't run in CI any more, I had to try another tack. I could also have picked a globbing library (any library, or the one ESLint uses), and that would have substituted some of the code I wrote, but I suspected we didn't need the long tail of features that some of those libraries provide. I might be wrong about that though.

Funny thing is, the previous implementation wasn't as simple as it seemed either: it was a very delicate working around the idiosyncrasies of how the underlying tools work (ie. different glob semantics, different treatment of ignore files, how they behave when circumstances change, like a file list ends up being empty etc) combined with our own self-imposed complications (like the way we needed to run in projects and at the top level, or we wanted to lint all ".js" but not really all ".js") — basically, all of that added up to something very fragile (full diff is here). If you look at the pull I sent Brian) you can see how a few of those complications actually got simplified in liferay-portal (simpler globs in the top-level npmscripts.config.js, for example, and in the ignore files, and greater coverage).

Trying to look on the bright side of this change that definitely brings more complexity than I would prefer, doing it ourselves opens up a few things: we get to turn our checks back on in CI again; it runs twice as fast; we might be able to use the Prettier NodeJS API to get even JS inside of JSP templates formatted (although I know we have high hopes that we won't have so much JS in JSP in the future); and we also address another source of complaint that I got from non-frontend folks which was that the Prettier output was too noisy.

It seems like as we continue down this road that we will end up just writing csf over again.

That might eventually happen, because we know Brian wants some changes to how Prettier formats code that can't be made via configuration alone. But the good news is that even if we do do that (whether it be forking, or plugins, or monkey patching, or — unlikely — getting a PR accepted upstream) the tool won't be a regular-expression-powered, string-chopping-and-gluing tool like csf, but a robust AST-driven utility which hopefully won't be too much of a maintenance burden for us.

Thanks for being a voice against complexity @bryceosterhaus. We need to be careful, and hopefully I haven't gone much too far this time (only a little too far!).

[bryceosterhaus]

Thanks for further clarity. I'm sure we are both on the same side and agree, its sort of a necessary complexity at the moment. Especially because of CI, thats a pretty big reason.

That might eventually happen, because we know Brian wants some changes to how Prettier formats code that can't be made via configuration alone.

Different conversation for this probably at a later date, but IMO we need to push back on this as much as we can. It seems like a waste of time to fork and create our own rules just to appease subjective preference, especially when it comes to things like "is else { on the same line or new line as the previous }". :emote for ending my opinions:

Thanks again for the response though, it might even be helpful to add this into the docs of this tool. Always good to have explanations on the need for complexity for future devs or may try to refactor.

[wincent]

we need to push back on this as much as we can

Yep, we won't rush to fork, if we can avoid it. Part of the reason we've been able to use Prettier at all is that @jbalsas effectively was able to negotiate a trial period for it, with the condition of addressing any remaining concerns by the end of 2019. As the year progresses, the benefits (more consistency, faster formatting, less time spent dealing with formatting bugs etc) will hopefully become clear, and we'll see what concerns (like the ones you mention) remain. We'll want to avoid the avoidable work and do only the unavoidable work, if there is any.