Skip to content

Commit

Permalink
Make RN Modules ESLint rule call into NativeModule spec parser
Browse files Browse the repository at this point in the history 
Summary:
This diff guts the React Native Modules ESLint rule, and makes it instead call into the NativeModule spec parser. After calling into the parser, the lint rule loops over all collected errors, and reports them.

## Benefits
- There is now one source of truth of what is a "correct" NativeModule spec: The NativeModule spec parser.
- Every change we make to the NativeModule spec parser will reflect in the lint rule. We have a number of changes planned for the NativeModule parser that will change what it means for a NativeModule spec to be correct. These changes now won't have to be duplicated in the ESLint rule.
- The linter will *never* show any false positive errors. If there's an error in the linter, you *need* to fix it. Otherwise, the codegen *will* fail. This is huge. Previously, people were used to ignoring the linter, because it over-reported errors. This behavior won't slide after this stack lands.

NOTE: This will run the NativeModules parser on all our NativeModule specs. We may have to check the specs, or the parser. Therefore, this could take some time to land.

## How does the lint rule work now?
In every JavaScript file, the ESLint rule looks for `CallExpression` AST Nodes. Once it detects a `CallExpression` that corresponds to a `TurboModuleRegistry.get` or `TurboModuleRegistry.getEnforcing`  it:
  1. Marks the file as a TurboModule spec
  2. Validates the `CallExpression` to verify that it is called with the <Spec> type parameters (i.e: like this: TurboModuleRegistry.get<Spec>(...)).

When we're done the visition for the JavaScript file (i.e: in `Program:exit`), if the JavaScript file was a TurboModule spec:
1. Report a lint error if the filename doesn't start with Native.
2. We parse the source using flow-parser, and run the NativeModule spec parser on it. We capture all ParserErrors and report them via ESLint.

## When can I start using this lint rule in VSCode?
I made the ESLint VSCode plugin use flow-node in D24454702. It'll take 3-4 weeks for this change to be shipped to everyone's VSCode. After the update, the linter will automatically start working in VSCode. Until then, we'll only get feedback on Sandcastle/Landcastle for lint rule violations.

Changelog: [Internal]

Reviewed By: fkgozali

Differential Revision: D24379783

fbshipit-source-id: 222778d8a84d7010eb7b3ad71b34a7fe1f52e509
  • Loading branch information
@RSNara @facebook-github-bot
RSNara authored and facebook-github-bot committed Oct 24, 2020
1 parent 2ff1d4c commit ad5802c
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 653 deletions.
321 changes: 10 additions & 311 deletions packages/eslint-plugin-codegen/__tests__/react-native-modules-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,305 +18,39 @@ const NATIVE_MODULES_DIR = __dirname;

const eslintTester = new ESLintTester();

const VALID_SPECS = [
// Standard specification will all supported param types.
{
code: `
'use strict';
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(): void,
func2(a: number, b: string, c: boolean): void,
func3(a: Object, b: Array<string>, c: () => void): void,
func4(a: ?string): void,
func5(a: ?Object, b: ?Array<string>, c: ?() => void): void,
func6(a: string[], b: ?number[]): void,
func7(): string,
func8(): Object,
func9(): Promise<string>,
func10(): number,
func11(): boolean,
func12(a: {|x: string|}): void,
func13(a: $ReadOnlyArray<Object>): void,
func14(): {|
x: number,
y: string,
|},
a: number,
b: string,
c: {a: number, b: string},
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
},

// With convenience API wrapper
{
code: `
'use strict';
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(a: Object): void,
}
const NativeModule = TurboModuleRegistry.get<Spec>('XYZ');
const NativeXYZ = {
func1(a?: number): void {
NativeModule.func1(a || {});
},
};
export default NativeXYZ;
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
},

// Non-spec JS file.
{
code: `
'use strict';
import {Platform} from 'react-native';
export default Platform.OS;
`,
},
];
const VALID_SPECS = [];

const INVALID_SPECS = [
// Haste module name doesn't start with "Native"
// Untyped NativeModule require
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(): void,
func1(a: string): {||},
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/XYZ.js`,
errors: [
{
message: rule.errors.invalidHasteName('XYZ'),
},
],
},

// Invalid Spec interface name
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Foo extends TurboModule {
func1(): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.invalidNativeModuleInterfaceName('Foo'),
},
{
message: rule.errors.specNotDeclaredInFile(),
},
],
output: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
},

// Missing method in Spec
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.missingSpecInterfaceMethod(),
},
],
},

// Invalid Spec method return type
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(): XYZ,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedMethodReturnType('XYZ'),
},
],
},

// Unsupported Spec property
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
id: Map<string, number>,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('Map'),
},
],
},

// Unsupported nested Spec property
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
a: {
b: number,
c: () => number,
},
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('Function'),
message: rule.errors.misnamedHasteModule('XYZ'),
},
],
},

// Unsupported Spec method arg type
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
type SomeType = {};
export interface Spec extends TurboModule {
func1(a: SomeType): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('SomeType'),
},
],
},

// Unsupported Spec method arg type: optional
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(a?: string): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('optional string'),
},
],
},

// Unsupported Spec method arg type: unsupported nullable
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
type Foo = {};
export interface Spec extends TurboModule {
func1(a: ?Foo): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('nullable Foo'),
},
],
},

// Unsupported Spec method arg type: generic Function
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(a: Function): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('Function'),
},
],
},

// Unsupported Spec method arg type: nullable generic Function
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(a: ?Function): void,
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.unsupportedType('nullable Function'),
},
],
},

// Spec method object return type must be exact
// Untyped NativeModule require
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(a: string): {},
func1(a: string): {||},
}
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.inexactObjectReturnType(),
},
],
},

// Untyped NativeModule require
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export default TurboModuleRegistry.get('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.untypedModuleRequire('get'),
message: rule.errors.untypedModuleRequire('NativeXYZ', 'get'),
},
],
},
Expand All @@ -325,35 +59,15 @@ export default TurboModuleRegistry.get('XYZ');
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export default TurboModuleRegistry.get<>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.incorrectlyTypedModuleRequire('get'),
},
],
},

// Incorrectly typed NativeModule require: 1 type, but wrong
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
export interface Spec extends TurboModule {
func1(a: string): {||},
}
// According to Flow, this also conforms to TurboModule
type Spec1 = {|
getConstants: () => {...}
|};
export default TurboModuleRegistry.get<Spec1>('XYZ');
export default TurboModuleRegistry.get<>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.incorrectlyTypedModuleRequire('get'),
message: rule.errors.incorrectlyTypedModuleRequire('NativeXYZ', 'get'),
},
],
},
Expand All @@ -370,22 +84,7 @@ export default TurboModuleRegistry.get<Spec, 'test'>('XYZ');
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.incorrectlyTypedModuleRequire('get'),
},
],
},

// NativeModule spec not declared in file
{
code: `
import {TurboModuleRegistry, type TurboModule} from 'react-native';
import type {Spec} from 'NativeFoo';
export default TurboModuleRegistry.get<Spec>('XYZ');
`,
filename: `${NATIVE_MODULES_DIR}/NativeXYZ.js`,
errors: [
{
message: rule.errors.specNotDeclaredInFile(),
message: rule.errors.incorrectlyTypedModuleRequire('NativeXYZ', 'get'),
},
],
},
Expand Down
4 changes: 4 additions & 0 deletions packages/eslint-plugin-codegen/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,9 @@
"url": "git@github.com:facebook/react-native.git",
"directory": "packages/eslint-plugin-codegen"
},
"dependencies": {
"flow-parser": "^0.121.0",
"react-native-codegen": "*"
},
"license": "MIT"
}
Loading

0 comments on commit ad5802c

Please sign in to comment.