@stylistic/indent: regression in 2.12.0

[voxpelli]

Validations

• Follow our Code of Conduct
• Read the Contributing Guide.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.

Describe the bug

In the PR that updates neostandard to ^2.12.0 neostandard/neostandard#228 the canary tests are failing for 5 out of 26 users of neostandard, probably due to #625 / #621. The failing ones are:

• voxpelli/markdown-or-chalk
• voxpelli/buffered-async-iterable
• fastify/fastify
• hifiwi-fi/breadcrum.net
• nodejs/undici

A failure rate of ≈20% is too high for a bug fix I think, so my suggestion is to revert and/or fix #625.

Until that I will pin neostandard to 2.11.0.

Examples of failures:

It wants eg this in voxpelli/buffered-async-iterable:

    const bufferPromise = currentSubIterator
      ? Promise.resolve(currentSubIterator.next())
        .catch(err => ({
          err: err instanceof Error ? err : new Error('Unknown subiterator error'),
        }))
        .then(async result => {

To instead be this:

    const bufferPromise = currentSubIterator
      ? Promise.resolve(currentSubIterator.next())
          .catch(err => ({
            err: err instanceof Error ? err : new Error('Unknown subiterator error'),
          }))
          .then(async result => {

And eg this in fastify/fastify:

    payload = (serializerFn === false)
      ? serializeError({
        error: statusCodes[statusCode + ''],
        code: error.code,
        message: error.message,
        statusCode
      })

To instead be this:

    payload = (serializerFn === false)
      ? serializeError({
          error: statusCodes[statusCode + ''],
          code: error.code,
          message: error.message,
          statusCode
        })

Reproduction

neostandard/neostandard#228

Contributes

• I am willing to submit a PR to fix this issue
• I am willing to submit a PR with failing tests

[antfu]

Honestly, to me, this looks more like a fix rather than a regression. And it's indeed involved with preferences. If you prefer the old behaviour, I am up to introducing an option for that. /cc @9romise WDYT?

[voxpelli]

If you prefer the old behaviour, I am up to introducing an option for that

I think this change should be introduced as an option and become the default in the next major version.

I do understand that its a fix, but its not a fix for a recent regression and with it causing failures in ≈20% of projects that I have tested it on (not only my projects, fastify and undici as well) I think its too big of a change to introduce in a non-major version of a linting module.

The fact that I had to do neostandard/neostandard#230 to avoid the regression for neostandard users is indicative of that.

[voxpelli]

If you prefer the old behaviour, I am up to introducing an option for that

Though an option is better than nothing, as then I can at least ensure that the users of neostandard won't experience a regression, but its not because I prefer the old behaviour, it's because I prefer to limit breaking changes :)

[9romise]

To me, this is indeed a fix for the option, but it’s a very old bug to trace back.

We also need to consider users who have upgraded to 2.12.0 and accepted it as a bug fix.

If compatibility is necessary, I think turning offsetTernaryExpressions into an object with sub-options could be a solution. Since indent already has many options, and it's hard to classify the fix as a new option.

[voxpelli]

We also need to consider users who have upgraded to 2.12.0 and accepted it as a bug fix.

Considering that it's been less than 3 days since release that group is still quite small

Do you have any canary tests of your own to get a benchmark on how breaking it is?

[antfu]

Yeah, for this case, I think it's tricky to define what is a "breaking change", especially this about stylistic. I understand your point of view, but it would be tricky for us to do it when every fix can be a breaking change, as the old eslint behavior are there for so long and baked in every project.

I would try to define "breaking change" here as changes with incompatibility / changes on defaults. And would define "fix" as "things as they should be, but aren't before".

Consider the code snippets below:

A:

    payload = (serializerFn === false)
      ? serializeError({
          error: statusCodes[statusCode + ''],
          code: error.code,
          message: error.message,
          statusCode
        })

B:

    payload = (serializerFn === false)
      ? serializeError({
        error: statusCodes[statusCode + ''],
        code: error.code,
        message: error.message,
        statusCode
      })

Which one looks more "correct" to you? Or it's still about different preferences?

I think both "fix" or "breaking changes" would result in the existing codebase having to make changes. As long as they didn't change the preference, it should be a command of --fix to adopt, isn't it?

[antfu]

Or, to add another way of thinking, do you think that the existing projects were coded like that because of

• They actually prefer that style of indentation
  or
• ESLint/ESLint Stylistic weren't able to catch up that case
  ?

From our perspective, we are fixing a case that wasn't handled well before, considering it either a fix or feature, and releasing it as a minor. Of course, if this change causes a lot of trouble to the projects using it negatively, we are happy to revert that and figure out a smooth way.

But do you think the final result is more positive or negative?

[voxpelli]

I generally think that anything (fixes or features) that makes the default linting stricter or different is a breaking change unless it only affects rare edge cases (eg. <5% of users or something).

I would only do such breaking changes if if necessitated for security reasons or to revert a very recent breaking change.

(This is eg. why we decided to relax a rule in neostandard: To enable people to transition without forcing them to transition. neostandard/neostandard#143)

Its not a matter of whether the way is the more proper way, its about how its introduced. (I see benefits with both styles in this case, but I think I actually prefer the old way, as the double indention is a bit verbose)

[antfu]

Umm, interesting...

I generally think that anything (fixes or features) that makes the default linting stricter or different is a breaking change unless it only affects rare edge cases (eg. <5% of users or something).

That's a very interesting perspective. I personally think breaking change is a breaking change, no matter how much it affects. And making the linter stricter is way better than making it looser, as making it looser might end up having things sneak through without awareness.

Just sharing what I thought for reference/fun :P

---

But yeah, I think this down to different expectations/definitions between ESLint Stylistic and nanostandard - which I think your points also make total sense from the perspective of nanostandard, but I am also not sure if ESLint Stylistic should/would adopt to that mind set.

👋 I'd love to hear more perspectives from different individuals/projects to make the decision about whether we should revert to the default behavior. And improve our workflow/standard.

Either way, I'd be happy to introduce an option for the old behavior to make the case in nanostandard easier to adopt at least.

[voxpelli]

different expectations/definitions between ESLint Stylistic and your neostandard

In that neostandard prefers to not break the linting in minor releases?

Also: Its not just mine, its used by eg. fastify and undici as well

[antfu]

Note: I edited my comment changing from your neostandard to just neostandard at the same time as you comment. I am not a native English speaker, I am very sorry if my tone didn't sound as friendly due to my wording/mistake. I was mostly trying to differentiate different points of view between ESLint Stylistic and neostandard, but not trying to indicate anything personally. Sorry for the confusion.

[voxpelli]

No worries, my main point is that projects without lock files will possibly have their linting fail now, making all the PR:s etc there fail until I release a new version of neostandard that pins this dependency

[voxpelli]

One such example: voxpelli/buffered-async-iterable#40

[InvisibleGit]

@antfu I'd prefer option B in your example.

Personally I prefer eslint/@Stylistic over prettier since it's a lot less opinionated and much more configurable.

If there is a breaking change without the possibility of the user to opt-out it would be too opinionated.

Just one more opinion though...

Thanks to the whole team for their work!

[controversial]

I'd love to hear more perspectives from different individuals/projects to make the decision about whether we should revert to the default behavior

Personally, I considered the old behavior to be a bug, I’m glad it got fixed, and it took only a couple minutes to find the relevant PR in the stylistic/eslint-plugin changelog, understand what changed, and apply the auto-fix.

In my opinion the impact of this is very small, especially since the rule is auto-fixable. It should take most projects only a handful of minutes to resolve this change in linting behavior.

If the change were reverted now, people who’ve updated this package since yesterday and run the fix will have to change back, only to have to re-apply the change again in the future.

[antfu]

After careful consideration, I consider #625 an intended fix, NOT an unexpected breaking change. Especially considering the tests are purely additive (https://github.com/eslint-stylistic/eslint-stylistic/pull/625/files#diff-8db334dbea6f7708da212dad8b3a868ff569b0b0980b617ab5f52bebcf6eec4a), meaning that we neither documented the previous behavior, nor had that in our test spec (if we even consider tests as part of our docs).

Also, if taking our docs' examples into account, I believe the new version makes the behavior more consistent.

Thus I don't think we should revert that change.

To resolve the concerns of this thread, I created #636 that introduces an additional option to opt-in back the previous behaviors, as the comments above show this could be involved with preferences.

Will have this open while longer for more feedback. If there is no strong objection from the team members, I am planning to release a new version with #636 in two days.

[9romise]

makes the default linting stricter or different is a breaking change

It’s hard to define a breaking change for ESLint rules, especially for stylistic ones. When there’s a bug in a linting rule, fixing it inherently becomes a breaking change. Do you agree?

Coming back to this specific issue, the change affects how function calls are handled compared to arrow functions, objects, etc. when the offsetTernaryExpressions option is enabled. (In fact, they are expected to be consistent because they’re all ternary expressions.) From this point of view, there is no doubt that #625 is a fix for #621.

If the old style is preferred, offsetTernaryExpressions can be set to false. But this would also break another group of coding style. Either way, the current option can no longer satisfy the previous version’s behavior. The only thing we can do is introduce a new option for this, but is it really worth it? Personally, I currently lean towards not doing so.

unless it only affects rare edge cases (eg. <5% of users or something).

Judging whether something is a breaking change purely based on the number of downstream users it affects feels a bit one-sided to me, as it also entirely depends on their ESLint configurations and coding styles.

[voxpelli]

Lets add some historic context:

When ESLint 7.19.0 was released Jan 31, 2021, that regressed the behavior from A to B in a minor release, breaking eg. fastify: standard/standard#1702 (comment)

It was reported as a regression to ESLint in eslint/eslint#14058 (with me commenting in eslint/eslint#14058 (comment)) but with no action from the ESLint team until they closed it because they deprecated the style rules: eslint/eslint#14058 (comment)

Now almost 4 years later and at least two major versions later the regression is fixed in a minor release, yet again breaking eg. fastify.

So this is the second time that standard / neostandard has had to handle breaking changes in this rule – first the regression that we simply needed to accept eventually, causing some frustration from our users, and now 4 years later the same again by the regression getting fixed.

If the regression had been fixed in a timely manner it wouldn't have been a breaking change, but fixing it now 4 years later is as disruptive as the regression itself originally was.

---

I consider #625 an intended fix, NOT an unexpected breaking change

This is a false dichotomy – something can be a correct fix and yet still be a breaking change.

Especially considering the tests are purely additive, meaning that we neither documented the previous behavior, nor had that in our test spec (if we even consider tests as part of our docs).

So if ones code has a pattern that's not covered by the current test cases and documentation, then one should accept that ones linting may break with any new release of eslint-stylistic? How can one reasonably verify that the patterns of ones code is in line with the test cases of eslint-stylistic?

To resolve the concerns of this thread, I created #636 that introduces an additional option to opt-in back the previous behaviors, as the comments above show this could be involved with preferences.

This is good, but it should be an opt-out rather than an opt-in. This enables neostandard to avoid breaking the projects of our users, but its troubling that it accepts the breaking of projects with eg. a ^2.11.0 dependency version range towards eslint-stylistic, like the version prior to [email protected] had.

---

It’s hard to define a breaking change for ESLint rules, especially for stylistic ones. When there’s a bug in a linting rule, fixing it inherently becomes a breaking change. Do you agree?

Yes, so fixing regressions in a timely manner is key. Not fixing them 4 years later to repeat the same disruption that the original regression caused.

unless it only affects rare edge cases (eg. <5% of users or something).

Judging whether something is a breaking change purely based on the number of downstream users it affects feels a bit one-sided to me, as it also entirely depends on their ESLint configurations and coding styles.

This is an unfair representation of my comment:

I generally think that anything (fixes or features) that makes the default linting stricter or different is a breaking change unless it only affects rare edge cases (eg. <5% of users or something).

It all comes down to what constitutes "documented behavior":

• Is "documented behavior" only explicit documentation on a website or in a markdown file?
• Is tested behavior some kind of "documented behavior" as @antfu says in @stylistic/indent: regression in 2.12.0 #633 (comment)?
• Is the fact that a public API has consistently for years returned true for a mainstream set of inputs, forcing users to adapt to that behavior, also an implicit "documented behavior"?

If the documentation says that it should return false for a mainstream set of inputs, the actual code for years has instead returned true, the tests hasn't covered that specific set of inputs – what is the "correct" behavior? The documentation that doesn't match the reality or the reality?

it also entirely depends on their ESLint configurations and coding styles

When it comes to neostandard it doesn't, as that's a full shared config that generally isn't modified by users. This is the benefit of shared configs. Its something that can more easily be related to. Sadly "shared configs" is no longer a concept that ESLint embraces and promotes (see neostandard/neostandard#220), so its likely that it will become harder and harder to maintain such a one and as such also harder and harder for plugins like this one to have powerful canary tests (we could eg. work with you to add canary tests that proactively tests new eslint-stylistic against all the +150 modules that we test against, so that regressions can be caught proactively rather than retroactively)

[antfu]

I tried my best to relate to your use case and worked to find a middle ground so both ESLint Stylistic and your project could move on. It's ok for different projects to have different policies and versioning. I respect your choice so I brought the option that makes your maintenance a bit easier.

I am once again sorry if this change caused any inconvenience to you, but please also understand this world is not only black and white. I would wish we could use more friendly words to communicate in the future.

[voxpelli]

I'm simply struggling in finding what the difference in policy is.

Is the following your policy?

If ones code has a pattern that's not included in the ESLint-stylistic test cases or documentation, then one should expect that ones linting can break with any minor and patch release of eslint-stylistic?

[controversial]

I think the policy being applied here is more fairly expressed along the lines of

if some code passes linting due to a bug in a rule, that code may begin failing after a bug fix in a semver-minor update.

As @9romise pointed out,

When there’s a bug in a linting rule, fixing it inherently becomes a breaking change.

Do you think all user-visible changes to linting behavior should be kept out of minor versions? Where do you draw the line for what’s “too breaking”?

[voxpelli]

@controversial That more fair expression relies on a definition of what constitutes a bug in a rule though. How does such a bug manifest itself to the user so that the user is aware that they are relying on a buggy behavior?

Do you think all user-visible changes to linting behavior should be kept out of minor versions?

I think that everything that causes code that previously passed linting to no longer pass linting should be restricted to major releases unless the rule is so broken that no sensible code will ever pass it.

As shown in this thread its not clear that the result in A is unintended – some even prefers it over B.

[voxpelli]

This is a discussion more suitable for other mediums though such as blogs (or Bluesky as preferred by eg. @antfu), so lets move it away from here.

As a closing note though:

To help neostandard / standard know what's a bug and what isn't there's a long tradition within those projects to run automated canary / compatibility tests. neostandard currently runs 118 on every code change. Perhaps that's the difference in "philosophy" here? We know when things break for our users, while this project doesn't?

[9romise]

Obviously, some people also believe that this is the correct fix.
If your request is only related to style preference, then I think it is a reasonable and worthwhile one to help solve. But it seems that it is not your starting point.

You have always emphasized "break". So, in your opinion, does any update to an ESLint plugin that causes users to fail linting count as a "break"?

If fixing the erroneous logic in a rule is also considered a "break", the work of the ESLint plugin will be difficult to continue.

Rules are designed to find and fix problems. When a rule itself has a bug, how do you think we can fix it without breaking the original linting results?

[voxpelli]

You have always emphasized "break". So, in your opinion, does any update to an ESLint plugin that causes users to fail linting count as a "break"?

If fixing the erroneous logic in a rule is also considered a "break", the work of the ESLint plugin will be difficult to continue.

I think I answered this here? #633 (comment)

Rules are designed to find and fix problems.

For style linting, it's to find and enforce a specific coding style, not to find and enforce problematic logic.

When a rule itself has a bug, how do you think we can fix it without breaking the original linting results?

You can:

• Make the fix opt in within a minor version and make it default in the next major
• You can accept both the old and the new style in the minor version – loosening the linting – and making it strict in next major
• You can ship a new major version, which would clearly communicate that one can't expect one's projects to keep passing with the new version
• If you can make a case that the current behavior isn't relied on by anyone as it's so clearly wrong that users will have opted out of it already, then you can ship in a minor (eg if there would have been no indentation at all in A rather than just a different aligning)