[eslint-config-universe] Add more lint rules for TypeScript

[wschurman]

Why

In a recent universe PR (https://github.com/expo/universe/pull/4068) upstreaming some of the newly added TypeScript and import order rules was discussed. This PR adds/updates the following lint rules:

• Updated import/order to alphabetize imports and add newlines between import type groups. Reasoning is that this seems to be the preferred import convention from recent code reviews.
• ban-types - Disallow non-primitive boxed object types in typescript, which are explicitly discouraged in the TypeScript docs: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#general-types
• array-type - Enforce using the [] array specification syntax for TypeScript. - https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/array-type.md
• no-extra-non-null-assertion - Warns when something like variable!! is used when variable! is sufficient (thanks kotlin). - https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-extra-non-null-assertion.md
• no-for-in-array - Errors when using for...in-loop for an array, which is nearly always not what a develope means to do, as it iterates over the string properties of the Array object. - https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-for-in-array.md
• no-throw-literal - It seems to be commonly accepted that it's better to throw a Error or a subclass of Error - https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-throw-literal.md
• prefer-nullish-coalescing - Warns when a nullish coalescing operator (new in TS 3.7) could be used, added to speed up code reviews - https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md
• prefer-optional-chain - Warns when an optional chain can be used (new in TS 3.7), added to speed up code reviews - https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-optional-chain.md

How

Rules copied from universe repo, used yarn link to link local updated version of eslint-config-universe into universe, rules removed from universe local config.

Some of the added rules require type information from the TypeScript parser (part of @typescript-eslint/parser). Forcing this would be a breaking change, so to avoid that (and only need to increment the minor version number), the rules that require the type information were siloed into their own config extension and instructions were added to the README about how to utilize them. Version was bumped from 2.1.1 to 2.2.0 based on semver guidelines.

Looking for feedback on:

1. Whether all these rules make sense to be in the upstreamed config.
2. Strategy for keeping this as a non-breaking change vs just making a breaking change and forcing the extra update to the user's eslintrc.
3. Clarity of README updates.

Test Plan

1. Manually test the upstreamed rules using the yarn link method above and manually breaking the rules in universe code.
2. yarn test, verifying that the snapshots are generating the correct output.

[ide]

Splitting out the config that uses type analysis looks good. I think we should rename "universe/shared/typescript-parsed-linting" to something else for a couple small reasons:

• "linting" is implied: we don't name the other configs "web-linting", for example
• "parsed" is also implied: the linter always parses

some suggestions: typescript-analysis, typescript-typed

[ide]

Does this rule need the TS parser? Can it go under the regular rules section?

[wschurman]

Going to remove this for now as it causes too many errors in the expo packages.

[ide]

Also after this commit we'll need to re-lint all the files. (Might want a small zsh script for this.)

[wschurman]

Made updates based on code review and fixed lint. To ease review, this PR has 6 commits:

1. Base commit containing new configs
2. Autofix commit created using new flag, expotools check-packages --no-build --no-test --fix-lint
3. Manual fixes that couldn't be autofixed
4. Updates from code review.
5. Auto committing built file changes
6. Manual fixes for expotools check-packages

Also lol codeowners really owned this PR.

[ide]

LGTM (consider this a soft approval) with just a few comments left inline. Tests look good (failing iOS test is unrelated).

[ide]

Is this a new change? Code from src/<X> should end up in build/<X>.

[wschurman]

Yep. It's caused by the following change in packages/expo-gl/src/GLView.tsx:

- const packageJSON = require('../package.json');
+ import packageJSON from '../package.json';

Because of this and how typescript emits, the emitted package.json appears in the toplevel directory (build/) and the rest of the source must be one level deeper, so it looks like tsc automatically chose src as the subdirectory name.

This conversion was necessary because the import order linter warned about a relative import being out of order when it was places below other imports, but when placed above other imports a different linter warned that a const assignment appeared above an import (code appearing above import). Alternatively, we could disable the linter for this file, but from reading README the philosophy seemed to be fairly against the disabling.

[ide]

This makes sense but I don't think that shipping code under build/src for some packages (specifically ones that import package.json) and under build for others is a good idea. It's going to create confusion when different packages are structured differently, and when adding or removing the import can completely change a package's build path!

Philosophically, the intent of the linter is to reduce the attention cost of writing code. If the linter ends up adding new warnings that we need to disable except in very rare cases, we're just moving attention cost around instead of reducing it. Ideally we'd be able to keep the old require() call in this case.

The ESLint import order plugin says, "Statements using the ES6 import syntax must appear before any require() statements." This sounds different than the behavior you were seeing, but if we could get the import order plugin to have this behavior it sounds like we'd be able to use require() and address this issue.

[wschurman]

import-js/eslint-plugin-import#1626