ni / nimble

The NI Nimble Design System
https://nimble.ni.dev
MIT License
29 stars 9 forks source link

Avoid phantom diffs from prettier-eslint #2214

Closed m-akinc closed 5 days ago

m-akinc commented 6 days ago

Pull Request

🤨 Rationale

Running npm run format would result in git detecting that combobox/select.foundation.spec.ts had been modified, but the diffs would show no changes.

👩‍💻 Implementation

The problem was that formatting was replacing some CRLF line endings with LF line endings.

prettier-eslint runs prettier followed by eslint to fix any formatting issues. The xxx.foundation.spec.ts files each had some lines like this:

const {
    element, connect, disconnect, option1, option2, option3
} = await setup();

prettier would reformat those lines like this, because they could fit on a single line without exceeding the line length limit:

const { element, connect, disconnect, option1, option2, option3 } = await setup();

Then eslint would run, and because these objects have more than five properties (the limit configured by the NI styleguide), it inserts newlines next to the curly braces, so it looks like the original file, except that the inserted newlines are Unix-style (LF) rather than Window-style (CRLF). This causes Git to detect a diff, but it's invisible. Even if you would submit the change, Git normalizes all the line endings to Unix-style upon check-in anyway, so there's no actual, commitable change.

Solution

Ideally, we could configure eslint to use whatever line ending type is used in the rest of the file, but it seems to only let you specify Unix or Windows. Instead, I am now reconfiguring the object-curly-newline rule to use an arbitrarily large minProperties value, so that eslint does not introduce newlines just because there are a certain number of properties in an object. prettier will still introduce newlines as needed, based on line length.

I also updated the npm format scripts to stop calling eslint-fix before prettier-fix. The latter already does both a prettier -fix and eslint -fix, so the separate eslint-fix is redundant.

🧪 Testing

Ran npm run format on the whole repo, and only the two problem files were changed.

✅ Checklist

m-akinc commented 5 days ago

@atmgrifter00 I removed the file (Angular example app file) that pulled you in as a code owner. Nothing left for you to review now.