Use at() instead of direct access

[ShridharGoel]

Expensify/App#43055

[fabioh8010]

Some things I noticed in this PR:

1. Tests in the repo are failing. One is related to tests/use-periods-error-messages.test.js file (which I think we don't need to fix right now), and for the other one we need to upgrade jest to latest version (29.7.0) in order to fix it, we could do in this PR.
2. We need a good way to determine if we are dealing with a object or an array, since I can use the same [] notation to access object's properties but we don't want the rule to apply to objects. I think we can use the parser services from @typescript-eslint/utils to help us identify an array in the project, for example something like this.

   const { ESLintUtils } = require("@typescript-eslint/utils");
   ...
   function checkNode(node) {
      const parserServices = ESLintUtils.getParserServices(context);
      const typeChecker = parserServices.program.getTypeChecker();
   
      function isArrayType(node) {
            const type = typeChecker.getTypeAtLocation(
               parserServices.esTreeNodeToTSNodeMap.get(node)
            );
            return typeChecker.isArrayType(type) || typeChecker.isTupleType(type);
      }
   ...

   You will also need to use RuleTester from @typescript-eslint/rule-tester in your test file. Here is an interesting doc.

[ShridharGoel]

Is const { ESLintUtils } = require("@typescript-eslint/utils"); working for you? I keep getting the below issue:

Cannot find module 'eslint/use-at-your-own-risk' from 'node_modules/@typescript-eslint/utils/dist/ts-eslint/eslint/LegacyESLint.js'

    Require stack:
      node_modules/@typescript-eslint/utils/dist/ts-eslint/eslint/LegacyESLint.js
      node_modules/@typescript-eslint/utils/dist/ts-eslint/ESLint.js
      node_modules/@typescript-eslint/utils/dist/ts-eslint/index.js
      node_modules/@typescript-eslint/utils/dist/index.js
      eslint-plugin-expensify/prefer-at.js
      eslint-plugin-expensify/tests/prefer-at.test.js

[fabioh8010]

@ShridharGoel you must upgrade jest to fix this:

Tests in the repo are failing. One is related to tests/use-periods-error-messages.test.js file (which I think we don't need to fix right now), and for the other one we need to upgrade jest to latest version (29.7.0) in order to fix it, we could do in this PR.

[ShridharGoel]

Thanks, but consistently getting this issue:

 A fatal parsing error occurred: Parsing error: ESLint was configured to run on `<tsconfigRootDir>/estree.ts` using `parserOptions.project`: <tsconfigRootDir>/tsconfig.json
    However, that TSConfig does not include this file. Either:
    - Change ESLint's list of included files to not include this file
    - Change that TSConfig to include this file
    - Create a new TSConfig that includes this file and include it in your parserOptions.project

In the test file, this is included:

const ruleTester = new RuleTester({
    parser: require.resolve('@typescript-eslint/parser'),
    parserOptions: {
        project: './tsconfig.json',
        sourceType: 'module',
        ecmaVersion: 2020,
        tsconfigRootDir: __dirname,
    },
});

tsconfig.json:

{
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5",
    "sourceMap": true
  },
  "exclude": [
    "node_modules"
  ]
}

I've also tried adding .eslintignore.

[ShridharGoel]

@fabioh8010 Can you have a look at the above?

[fabioh8010]

@ShridharGoel

• Add @typescript-eslint/rule-tester dependency to project.
• Place the tsconfig.json file under eslint-plugin-expensify/tests/ folder.
• Create a empty file.ts file insider eslint-plugin-expensify/tests/ folder – it's necessary for testing TS.
• Adjust the test code like this:

  const RuleTester = require('@typescript-eslint/rule-tester').RuleTester;
  const rule = require('../prefer-at');
  const message = require('../CONST').MESSAGE.PREFER_AT;
  
  const ruleTester = new RuleTester({
      parser: '@typescript-eslint/parser',
      parserOptions: {
          project: './tsconfig.json',
          tsconfigRootDir: __dirname,
          sourceType: 'module',
          ecmaVersion: 2020,
      },
  });
  
  ...

Please let me know if it works or not.

[ShridharGoel]

It did not work:

Cannot find module '@typescript-eslint/utils/eslint-utils' from 'node_modules/@typescript-eslint/rule-tester/dist/RuleTester.js'

Also, it is in JS.

So do we need TS compatibility here?

[fabioh8010]

@ShridharGoel I don't understand your comment, please provide more information about the error:

• Where it happens?
• When it happens?
• Do you mind sharing your changes in a diff so I can assist you better?

[ShridharGoel]

@fabioh8010 The test suite fails with the above message.

I've pushed the changes in this branch itself: baf7d27

[fabioh8010]

Thanks @ShridharGoel , will take a look tomorrow!

[fabioh8010]

@ShridharGoel I checked your PR and as I said before you need to upgrade jest in order to fix the errors when running the tests, you can upgrade by running this command npm install jest@latest. Please upgrade and let me know if it works.

[ShridharGoel]

@fabioh8010 Thanks, I had missed that. It is working now, can you check the updated code?

[fabioh8010]

Looking good, left few comments!

[ShridharGoel]

Updated but it wouldn't pass the a[index] test. Will need some more time to check that.

[ShridharGoel]

@fabioh8010 @eh2077 Can you have a look?

Edit: Some extra files got added, I'll remove them.

[fabioh8010]

My final comments I believe, looks really great now 🚀 Please remove the additional files and I will approve

[fabioh8010]

two minor comments, besides that LGTM 🚀

[ShridharGoel]

Updated.

[eh2077]

I'll take a look and test it soon

[eh2077]

LGTM, just a small comment

[roryabraham]

do you think we could also verify that this case with as const is considered valid?

const myArr = ['a', 'b', 'c'] as const;
myArr[0]; // good, type should be `a`
myArr[1]; // good, type should be `b`
myArr[2]; // good, type should be `c`
myArr[3]; // error

[roryabraham]

@ShridharGoel heads-up that I am unable to merge this because it contains unsigned commits

[roryabraham]

can you please add .idea to the gitignore?

[ShridharGoel]

Updated.

[ShridharGoel]

Added .idea and *.iml files to gitignore.

[github-actions]

🚀 Published in 2.0.56

[carlosmiceli]

@ShridharGoel @roryabraham question: we've been trying with @MonilBhavsar to bump the eslint version in the App but we've been getting this error, which seems to be related to this rule added here? Any advice on how we should proceed?

[ShridharGoel]

I'll be bumping the ESlint version soon, and will handle it then.

…

Message ID: ***@***.*** .com>

[carlosmiceli]

Got it, would you please let me know when it's ready? thank you!

[carlosmiceli]

@ShridharGoel small bump :)

[ShridharGoel]

Fix has been identified, opening the PR in few minutes.

[ShridharGoel]

#112

[carlosmiceli]

@ShridharGoel Merged, thank you! Question though, do you know why this may be failing still with these period rules? I can see that the rule is updated already here and tests as well.

[carlosmiceli]

@ShridharGoel @roryabraham I see that the version hasn't been bumped yet and it's been over a week since this comment, do we know when are we going to do this? It's holding this PR, but not sure if there's something that's holding it back that I'm unaware of 🙏

[ShridharGoel]

Will be doing it soon. I don't have access to a Mac since few days, and this needs many manual changes as well so setting up WSL. But it shouldn't be a blocker, you can update the version I think without enabling this rule.