Repo: Ideas for avoiding drift between a renamed rule and the original

[kirkwaiblinger]

Suggestion

This is a followup to #8821 (comment)

In #8821, I was renaming a rule that has other active pull requests (#8688 and #8673). No matter which PR is merged first, the other PRs will have conflicts (that git probably will not detect as conflicts).

To prevent these semantic merge conflicts/drift, I had originally tried to bring the original and renamed rules into common code, to ensure runtime parity, and just fix up the metadata details when exporting the rule object for each file (see original commit: 12182f5). (I also did a similar approach for the test code).

This has one major upside (runtime parity), but several major downsides:

• more complicated and tedious code
• the resulting code circumvents some internal lint rules that ordinarily run on the test cases
• it's more tedious to introduce intentional drift if need be

We ended up deciding to go for the copypasta approach, and we'll just have to pay close attention to ensure that the changes to the rule get duplicated to the renamed version.

This got me, thinking though - is there a way we can be clever and have the ease of copypasta but also some CI-validated insurance against drift?

My thought was that we could consider making a CI-validated diff snapshot between the relevant files, so that drift would need to be explicitly accepted by updating said snapshot.

This issue is to consider poking around and see if we can lay down a pattern for rule renames that have some CI/PR-validated drift prevention (via common code source or diff drift prevention). Another rule that this would apply to is no-throw-literal/only-throw-error. Maybe there are others, too.

The consensus may well also be that this is more effort to even think about than it's worth, no worries if so, we can just close it 😄

---

Very primitive sketch of the concept for the diff idea (but that would need significant refinement to make usable):

import { execSync } from 'child_process';
import * as path from 'path';

describe('Renamed rules should have stable diffs compared to their original versions', () => {
  it('(no-unnecessary-template-literals -> no-unnecessary-template-expression) rule files', () => {
    let diffString: string | undefined;
    try {
      execSync(
        `git diff --raw --no-index "${path.join(
          __dirname,
          '../src/rules/no-useless-template-expression.ts',
        )}" "${path.join(
          __dirname,
          '../src/rules/no-useless-template-literals.ts',
        )}"`,
      );
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
    } catch (error: any) {
      diffString = error.stdout.toString();
    }
    if (diffString == null) {
      throw new Error();
    }

    expect(diffString).toMatchSnapshot();
  });
  it('(no-unnecessary-template-literals -> no-unnecessary-template-expression) test files', () => {
    let diffString: string | undefined;
    try {
      execSync(
        `git diff --no-index "${path.join(
          __dirname,
          'rules/no-useless-template-expression.test.ts',
        )}" "${path.join(
          __dirname,
          'rules/no-useless-template-literals.test.ts',
        )}"`,
      );
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
    } catch (error: any) {
      diffString = error.stdout.toString();
    }
    if (diffString == null) {
      throw new Error();
    }

    expect(diffString).toMatchSnapshot();
  });
});

[bradzacher]

I think an existing rule should be able to be written as something like

export default Object.assign(
  {},
  renamedRule,
  {
    name: "old-name",
    meta: Object.assign(
      {},
      renamedRule.meta,
      {
        deprecated: true,
        replacedBy: ['new-rule-name'],
      },
    ),
  },
);

and everything should work as expected then. It'll cause syntactic conflicts with anyone trying to modify the old file and it'll ensure we only have one copy of the file in the codebase.

[kirkwaiblinger]

@bradzacher
Nice! That basically worked perfectly. (Only trivial change was the doc link)

import noUselessTemplateExpression from './no-useless-template-expression';

export default {
  ...noUselessTemplateExpression,
  ...{
    name: 'no-useless-template-literals',
    meta: {
      ...noUselessTemplateExpression.meta,
      ...{
        deprecated: true,
        replacedBy: ['no-useless-template-expression'],
      },
      docs: {
        ...noUselessTemplateExpression.meta.docs,
        url: 'https://typescript-eslint.io/rules/no-useless-template-literals',
      },
    },
  },
};

The only thing this doesn't address, then, is the copypasta of the tests.... But maybe that's not so important as long as the rule is written in this way? What do you think?

[bradzacher]

on small fix that replacedBy should be replacedBy: [renamedRule.name]
could make this a util for future renaming efforts.

[kirkwaiblinger]

I like that!

fun fact - our rules don't actually have the name property right now 😮 (at runtime or in the types; it gets used in the url creation then immediately dropped). Will take some slightly tedious tweaking of RuleCreator types to get this working.

[bradzacher]

Oh I thought that we started persisting the name for our doc gen infra.