[require-hook] what should we be doing for const variables?

[G-Rath]

Currently require-hook considers const valid to have outside of hooks and test methods; this is to allow this sort of code:

const { S3 } = require('@aws-sdk/client-s3');
const { mocked } = require('ts-jest/utils');

jest.mock('@aws-sdk/client-s3');

const mockS3 = mocked(S3, true);

const buildApiGatewayEvent = (message: string): APIGatewayProxyEvent => {
  return {
    body: JSON.stringify({ message }),
    headers: {}
  } as APIGatewayProxyEvent;
};

However, this also allows doing things like this:

describe('when the body is not valid json', () => {
  const event = buildApiGatewayEvent('');

  event.body = ''; // mutation!

  it('returns Bad Request', async () => {
    await expect(
      handler(event, fakeContext, console.log)
    ).resolves.toHaveProperty('statusCode', 400);
  });
});

Overall this isn't the worst thing in the world but it can be a nasty bug and part of what require-hook is meant to be helping you avoid; it is made worse if you consider that something in a test might be mutating the value:

const event = buildApiGatewayEvent('');

describe('when the body is not valid json', () => {
  it('returns Bad Request', async () => {
    event.body = ''; // mutation!

    await expect(
      handler(event, fakeContext, console.log)
    ).resolves.toHaveProperty('statusCode', 400);
  });
});

describe('when the body is valid json', () => {
  it('returns Ok', async () => {
    await expect(
      handler(event, fakeContext, console.log)
    ).resolves.toHaveProperty('statusCode', 200);
  });
});

In the above, the second test will fail due to event.body having been made blank in the previous test (assuming that test runs, which makes it even more confusing since if you stick a .only it'll suddenly start passing); ideally instead the event variable should be replaced with this:

let event;

beforeEach(() => {
  event = buildApiGatewayEvent('');
});

That way there's a new event every run.

---

I'm reasonably confident in our ability to detect if a const assignment should be in a hook (or specifically, that it should not be) due to the reasonably low number of patterns that actually should be allowed, so currently am proposing we do one of the following:

1. Continue to always ignore const variables, and assume the developer understands the implications (or lack of) for objects & arrays
2. Continue to always ignore const variables, with an option to consider const to be an error (excluding "special cases")
3. Consider const to be an error (excluding "special cases"), with an option to ignore const variables
4. ??? (something else entirely)

As to what we actually consider "special cases", I think function assignments are obvious, but for require and mocked it might actually be better to have another option that is an array of function call assignments to allow.

In terms of option names, I've not really got a decent proposal for the "allowed function call assignments to constant variables" array, but for the enabling/disabling I'm thinking something like ignoreConstantVariables/checkConstantVariables (depending on what our default is), or otherwise maybe an enum? (constantVariables: 'check' | 'ignore')

cc @SimenB

[G-Rath]

Thinking about it, I think it should be fine to also special case simple/primitive values e.g. numbers and strings since we can detect those easily and they are properly constant.