Skip to content

Commit

Permalink
feat: revise custom lint rule (#352)
Browse files Browse the repository at this point in the history 
We have a few too many instances of // eslint-disable-next-line custom-rules/valid-constructors 😅 .

It looks like this is mainly because the id field is static (for example the Stage parameter) or generated by the props passed in (for example InstanceType).

This PR revises the rules to be:
1. Private constructors don't get linted
2. Must be 1, 2 or 3 parameters
3. First parameter must be called scope
4. First parameter must be of type GuStack
5. If 2 parameters:
   - The second parameter must be called props
   - The second parameter must be a custom type
6. If 3 parameters:
   - The second parameter must be called id
   - The second parameter must be of type string
   - The third parameter must be called props
   - The third parameter must be a custom type
7. Only the third parameter can be optional or have a default value
  • Loading branch information
@akash1810
akash1810 authored Mar 29, 2021
1 parent 1e78c1d commit 5488958
Show file tree
Hide file tree
Showing 27 changed files with 247 additions and 194 deletions.
47 changes: 22 additions & 25 deletions docs/architecture-decision-records/002-component-constuctors.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ This project contains a large number of classes, making up the various construct

By having a consistent style for constructors, users will know what to expect every time they use a class from the library. This makes the developer experience easier and faster as users do not have to check what is required for each class.

2. Constrctors should be the best fit for each class
2. Constructors should be the best fit for each class

In the case of this library, this mainly means either missing out the id or changing the order of the id and props inputs. In scenarios where a sensible id can either be hardcoded or defaulted, this would prevent users from having to add ids everywhere, reducing the amount of code they have to write.

Expand All @@ -30,13 +30,13 @@ This project contains a large number of classes, making up the various construct

Constructors should follow the following rules for consistency.

1. Constructors should all accept a `scope` of type `GuStack` and an `id` of type `string`
1. The first parameter should be a `scope` of type `GuStack`:

:white_check_mark: Valid

```ts
class MyConstruct {
constructor(scope: GuStack, id: string) {
constructor(scope: GuStack) {
...
}
}
Expand All @@ -46,21 +46,21 @@ Constructors should follow the following rules for consistency.

```ts
class MyConstruct {
constructor(scope: Stack, id: string) {
constructor(scope: Stack) {
...
}
}
```

2. They can also take a `props` object which should be correctly typed
The construct/pattern will then have a static `id` as it will never change, for example the `Stage` parameter.

2. They can also take a `props` object which should be correctly typed:

:white_check_mark: Valid

```ts
interface MyConstructProps {...}
class MyConstruct {
constructor(scope: GuStack, id: string, props: MyConstructProps) {
constructor(scope: GuStack, props: MyConstructProps) {
...
}
}
Expand All @@ -70,24 +70,23 @@ Constructors should follow the following rules for consistency.

```ts
class MyConstruct {
constructor(scope: GuStack, id: string, props: object) {
constructor(scope: Stack, props: object) {
...
}
}
```

3. Where all `props` are optional, the `props` object should be optional as a whole
The construct/pattern will then derive `id` from `props` as it will never change, for example `InstanceTypeFor${props.app}`.

3. They can also take an `id` of type string and a `props` object which should be correctly typed

:white_check_mark: Valid

```ts
interface MyConstructProps {
prop1?: string;
prop2?: string
}
interface MyConstructProps {...}
class MyConstruct {
constructor(scope: GuStack, id: string, props?: MyConstructProps) {
constructor(scope: GuStack, id: string, props: MyConstructProps) {
...
}
}
Expand All @@ -96,19 +95,14 @@ Constructors should follow the following rules for consistency.
:x: Invalid

```ts
interface MyConstructProps {
prop1?: string;
prop2?: string
}
class MyConstruct {
constructor(scope: GuStack, id: string, props: MyConstructProps) {
constructor(scope: GuStack, id: any, props: object) {
...
}
}
```

4. Where `props` are optional, a default value for the `id` can be provided where appropriate
4. Where all `props` are optional, the `props` object should be optional as a whole

:white_check_mark: Valid

Expand All @@ -119,7 +113,7 @@ Constructors should follow the following rules for consistency.
}
class MyConstruct {
constructor(scope: GuStack, id: string = "MyConstruct", props?: MyConstructProps) {
constructor(scope: GuStack, id: string, props?: MyConstructProps) {
...
}
}
Expand All @@ -128,10 +122,13 @@ Constructors should follow the following rules for consistency.
:x: Invalid

```ts
interface MyConstructProps {...}
interface MyConstructProps {
prop1?: string;
prop2?: string
}
class MyConstruct {
constructor(scope: GuStack, id: string = "MyConstruct", props: MyConstructProps) {
constructor(scope: GuStack, id: string, props: MyConstructProps) {
...
}
}
Expand Down
234 changes: 124 additions & 110 deletions eslint/rules/valid-constructors.js
View file
Original file line number Diff line number Diff line change
@@ -1,38 +1,102 @@
// Rules
// 1. Must be at least 2 parameters
// 2. Can't be more than 3 parameters
// 3. First parameter must be called scope
// 4. First parameter must be of type GuStack
// 5. Second parameter must be called id
// 6. Second parameter must be of type string
// 7. Third parameter (if exists) must called props
// 8. If third parameter is present and non-optional then the second parameter should not be initialised
// TODO: 9. If all values in third type are optional then parameter should be optional
// 10. Third parameter type should be custom

const getPath = (node, path) => {
const keys = path.split(".");
let value = node;

for (const key of keys) {
if (!value) break;
value = value[key];
/*
RULES
1. Private constructors don't get linted
2. Must be 1, 2 or 3 parameters
3. First parameter must be called scope
4. First parameter must be of type GuStack
5. If 2 parameters:
- The second parameter must be called props
- The second parameter must be a custom type
6. If 3 parameters:
- The second parameter must be called id
- The second parameter must be of type string
- The third parameter must be called props
- The third parameter must be a custom type
7. Only the third parameter can be optional or have a default value
See `valid-constructors.test.ts` and `npm run test:custom-lint-rule`
TODO: If all values in third type are optional then parameter should be optional
*/

const NO_LINT_ERRORS = null;

const lintParameter = (param, node, context, { name, type, allowOptional, allowDefault, position }) => {
const hasDefault = param.type === "AssignmentPattern";
if (hasDefault && !allowDefault) {
return context.report({
node,
loc: param.loc,
message: `The ${position} parameter in a constructor cannot have a default`,
});
}

return value;
};
const isOptional = param.optional;
if (isOptional && !allowOptional) {
return context.report({
node,
loc: param.loc,
message: `The ${position} parameter in a constructor cannot be optional`,
});
}

const currentName = param.name ?? param.left.name;

if (currentName !== name) {
return context.report({
node,
loc: param.loc,
message: `The ${position} parameter in a constructor must be called ${name} (currently ${currentName})`,
});
}

const getStandardOrLeftProp = (node, path) => {
switch (node.type) {
case "Identifier":
return getPath(node, path);
case "AssignmentPattern":
return getPath(node, `left.${path}`);
default:
return false;
const tsType = param.typeAnnotation?.typeAnnotation.type ?? param.left.typeAnnotation.typeAnnotation.type;
const customType =
param.typeAnnotation?.typeAnnotation.typeName?.name ?? param.left?.typeAnnotation.typeAnnotation.typeName.name;

const currentType = type.startsWith("TS") ? tsType : customType;

if (currentType !== type) {
return context.report({
node,
loc: param.typeAnnotation.loc,
message: `The ${position} parameter in a constructor must be of type ${type} (currently ${currentType})`,
});
}
};

const scopeParamSpec = {
name: "scope",
type: "GuStack",
allowOptional: false,
allowDefault: false,
position: "first",
};

const idParamSpec = {
name: "id",
type: "TSStringKeyword",
allowOptional: false,
allowDefault: false,
position: "second",
};

const propsAsSecondParamSpec = {
name: "props",
type: "TSTypeReference",
allowOptional: false,
allowDefault: false,
position: "second",
};

const propsAsThirdParamSpec = {
name: "props",
type: "TSTypeReference",
allowOptional: true,
allowDefault: true,
position: "third",
};

module.exports = {
meta: {
type: "suggestion",
Expand All @@ -47,100 +111,50 @@ module.exports = {
create(context) {
return {
MethodDefinition(node) {
if (node.kind !== "constructor") return null;
if (node.kind !== "constructor") return NO_LINT_ERRORS;
if (node.accessibility === "private") return NO_LINT_ERRORS;

const params = node.value.params;

// 1. Must be at least 2 parameters
if (!Array.isArray(params) || params.length < 2) {
return context.report(
node,
params.loc,
"Construct or pattern constructors must take at least a scope and an id parameter"
);
}

// 2. Can't be more than 3 parameters
if (params.length > 3) {
return context.report(
node,
params.loc,
"Construct or pattern constructors can only take scope, id and props parameters"
);
}

const scope = params[0];

// 3. First parameter must be called scope
if (scope.name !== "scope") {
return context.report(
node,
scope.loc,
`The first parameter in a construct or pattern contructor must be called scope`
);
}

// 4. First parameter must be of type GuStack
if (scope.typeAnnotation.typeAnnotation.typeName.name !== "GuStack") {
return context.report(
node,
scope.typeAnnotation.loc,
`The first parameter in a construct or pattern contructor must be of type GuStack`
);
}

const id = params[1];

// 5. Second parameter must be called id
if (getStandardOrLeftProp(id, "name") !== "id") {
return context.report(
node,
id.loc,
`The second parameter in a construct or pattern contructor must be called id`
);
}

// 6. Second parameter must be of type string
if (getStandardOrLeftProp(id, "typeAnnotation.typeAnnotation.type") !== "TSStringKeyword") {
return context.report(
if (!Array.isArray(params)) {
return context.report({
node,
getStandardOrLeftProp(id, "typeAnnotation.loc"),
`The second parameter in a construct or pattern contructor must be of type string`
);
message: "Constructors must take at least one parameter",
loc: params.loc,
});
}

const props = params[2];

if (props) {
// 7. Third parameter (if exists) must called props
if (getStandardOrLeftProp(props, "name") !== "props") {
return context.report(
node,
props.loc,
`The third parameter in a construct or pattern contructor must be called props`
);
switch (params.length) {
case 1: {
const [scope] = params;
return lintParameter(scope, node, context, scopeParamSpec) ?? NO_LINT_ERRORS;
}
case 2: {
const [scope, props] = params;

// 8. If third parameter is present and non-optional then the second parameter should not be initialised
if (props.type !== "AssignmentPattern" && !props.optional && id.type === "AssignmentPattern") {
return context.report(
node,
id.loc,
`The second parameter cannot be initialised if there is a non-optional third parameter`
return (
lintParameter(scope, node, context, scopeParamSpec) ??
lintParameter(props, node, context, propsAsSecondParamSpec) ??
NO_LINT_ERRORS
);
}

// 10. Third parameter type should be custom
if (getStandardOrLeftProp(props, "typeAnnotation.typeAnnotation.type") !== "TSTypeReference") {
return context.report(
node,
getStandardOrLeftProp(props, "typeAnnotation.loc"),
`The third parameter in a construct or pattern contructor must be a custom type`
case 3: {
const [scope, id, props] = params;

return (
lintParameter(scope, node, context, scopeParamSpec) ??
lintParameter(id, node, context, idParamSpec) ??
lintParameter(props, node, context, propsAsThirdParamSpec) ??
NO_LINT_ERRORS
);
}
default:
return context.report({
node,
message: `Constructors can take a maximum of three parameters - there are ${params.length} params here!`,
loc: params.loc,
});
}

return null;
},
};
},
Expand Down
Loading

0 comments on commit 5488958

Please sign in to comment.