diff --git a/packages/grafana-ui/src/components/ThresholdsEditor/ThresholdsEditor.test.tsx b/packages/grafana-ui/src/components/ThresholdsEditor/ThresholdsEditor.test.tsx index ea94537c429..38cd8e5c763 100644 --- a/packages/grafana-ui/src/components/ThresholdsEditor/ThresholdsEditor.test.tsx +++ b/packages/grafana-ui/src/components/ThresholdsEditor/ThresholdsEditor.test.tsx @@ -161,7 +161,7 @@ describe('change threshold value', () => { }); describe('on blur threshold value', () => { - it.only('should resort rows and update indexes', () => { + it('should resort rows and update indexes', () => { const { instance } = setup(); const thresholds = [ { index: 0, value: -Infinity, color: '#7EB26D' }, diff --git a/scripts/cli/utils/useSpinner.ts b/scripts/cli/utils/useSpinner.ts index 81ed9bb6fcf..298a6516689 100644 --- a/scripts/cli/utils/useSpinner.ts +++ b/scripts/cli/utils/useSpinner.ts @@ -10,8 +10,7 @@ export const useSpinner = (spinnerLabel: string, fn: FnToSpin, killProcess await fn(options); spinner.succeed(); } catch (e) { - spinner.fail(); - console.log(e); + spinner.fail(e); if (killProcess) { process.exit(1); } diff --git a/scripts/grunt/default_task.js b/scripts/grunt/default_task.js index 8a71ea26627..a656e0c60af 100644 --- a/scripts/grunt/default_task.js +++ b/scripts/grunt/default_task.js @@ -34,14 +34,17 @@ module.exports = function (grunt) { ]); grunt.registerTask('no-only-tests', function () { - var files = grunt.file.expand('public/**/*_specs\.ts', 'public/**/*_specs\.js'); + var files = grunt.file.expand( + 'public/**/*@(_specs|\.test)\.@(ts|js|tsx|jsx)', + 'packages/grafana-ui/**/*@(_specs|\.test)\.@(ts|js|tsx|jsx)' + ); files.forEach(function (spec) { var rows = grunt.file.read(spec).split('\n'); rows.forEach(function (row) { if (row.indexOf('.only(') > 0) { grunt.log.errorlns(row); - grunt.fail.warn('found only statement in test: ' + spec) + grunt.fail.warn('found only statement in test: ' + spec); } }); }); diff --git a/style_guides/frontend.md b/style_guides/frontend.md index caef4f711ef..18069183e66 100644 --- a/style_guides/frontend.md +++ b/style_guides/frontend.md @@ -1,36 +1,36 @@ # Frontend Style Guide -Generally we follow the Airbnb [React Style Guide](https://github.com/airbnb/javascript/tree/master/react). +Generally we follow the Airbnb [React Style Guide](https://github.com/airbnb/javascript/tree/master/react). ## Table of Contents - 1. [Basic Rules](#basic-rules) - 1. [File & Component Organization](#Organization) - 1. [Naming](#naming) - 1. [Declaration](#declaration) - 1. [Props](#props) - 1. [Refs](#refs) - 1. [Methods](#methods) - 1. [Ordering](#ordering) +1. [Basic Rules](#basic-rules) +1. [File & Component Organization](#Organization) +1. [Naming](#naming) +1. [Declaration](#declaration) +1. [Props](#props) +1. [Refs](#refs) +1. [Methods](#methods) +1. [Ordering](#ordering) ## Basic rules -* Try to keep files small and focused and break large components up into sub components. +- Try to keep files small and focused and break large components up into sub components. ## Organization -* Components and types that needs to be used by external plugins needs to go into @grafana/ui -* Components should get their own folder under features/xxx/components - * Sub components can live in that component folders, so small component do not need their own folder - * Place test next to their component file (same dir) - * Component sass should live in the same folder as component code -* State logic & domain models should live in features/xxx/state -* Containers (pages) can live in feature root features/xxx - * up for debate? +- Components and types that needs to be used by external plugins needs to go into @grafana/ui +- Components should get their own folder under features/xxx/components + - Sub components can live in that component folders, so small component do not need their own folder + - Place test next to their component file (same dir) + - Component sass should live in the same folder as component code +- State logic & domain models should live in features/xxx/state +- Containers (pages) can live in feature root features/xxx + - up for debate? ## Props -* Name callback props & handlers with a "on" prefix. +- Name callback props & handlers with a "on" prefix. ```tsx // good @@ -56,5 +56,32 @@ render() { } ``` +- React Component definitions +```jsx +// good +export class YourClass extends PureComponent<{},{}> { ... } +// bad +export class YourClass extends PureComponent { ... } +``` + +- React Component constructor + +```typescript +// good +constructor(props:Props) {...} + +// bad +constructor(props) {...} +``` + +- React Component defaultProps + +```typescript +// good +static defaultProps: Partial = { ... } + +// bad +static defaultProps = { ... } +``` diff --git a/style_guides/pull-request-review-checklist.md b/style_guides/pull-request-review-checklist.md new file mode 100644 index 00000000000..2fd017386ea --- /dev/null +++ b/style_guides/pull-request-review-checklist.md @@ -0,0 +1,36 @@ +# Pull Request Review Checklist + +## High level checks + +- [ ] The pull request adds value and the impact of the change is in line with [Frontend Style Guide](https://github.com/grafana/grafana/blob/master/style_guides/frontend.md). +- [ ] The pull request works the way it says it should do. +- [ ] The pull request does not increase the Angular code base. + > We are in the process of migrating to React so any increment of Angular code is generally discouraged from. (there are a few exceptions) +- [ ] The pull request closes one issue if possible and does not fix unrelated issues within the same pull request. +- [ ] The pull request contains necessary tests. + +## Low level checks + +- [ ] The pull request contains a title that explains the PR. +- [ ] The pull request contains necessary link(s) to issue(s). +- [ ] The pull request contains commits with commit messages that are small and understandable. +- [ ] The pull request does not contain magic strings or numbers that could be replaced with an `Enum` or `const` instead. + +### Bug specific checks + +- [ ] The pull request contains only one commit if possible. +- [ ] The pull request contains `closes: #Issue` or `fixes: #Issue` in pull request description. + +## Frontend specific checks + +- [ ] The pull request does not increase the number of `implicit any` errors. +- [ ] The pull request does not contain uses of `any` or `{}` without comments describing why. +- [ ] The pull request does not contain large React component that could easily be split into several smaller components. +- [ ] The pull request does not contain back end calls directly from components, use actions and Redux instead. + +### Redux specific checks (skip if pull request does not contain Redux changes) + +- [ ] The pull request does not contain code that mutate state in reducers or thunks. +- [ ] The pull request uses helpers `actionCreatorFactory` and `reducerFactory` instead of traditional `switch statement` reducers in Redux. ([Redux framework](https://github.com/grafana/grafana/blob/master/style_guides/redux.md)) +- [ ] The pull request uses `reducerTester` to test reducers.([Redux framework](https://github.com/grafana/grafana/blob/master/style_guides/redux.md)) +- [ ] The pull request does not contain code that access reducers state slice directly, instead the code uses state selectors to access state. diff --git a/style_guides/redux.md b/style_guides/redux.md new file mode 100644 index 00000000000..ff64fe400f3 --- /dev/null +++ b/style_guides/redux.md @@ -0,0 +1,158 @@ +# Redux framework + +To reduce the amount of boilerplate code used to create a strongly typed redux solution with actions, action creators, reducers and tests we've introduced a small framework around Redux. + +`+` Much less boilerplate code +`-` Non Redux standard api + +## New core functionality + +### actionCreatorFactory + +Used to create an action creator with the following signature + +```typescript +{ type: string , (payload: T): {type: string; payload: T;} } +``` + +where the `type` string will be ensured to be unique and `T` is the type supplied to the factory. + +#### Example + +```typescript +export const someAction = actionCreatorFactory('SOME_ACTION').create(); + +// later when dispatched +someAction('this rocks!'); +``` + +```typescript +// best practices, always use an interface as type +interface SomeAction { + data: string; +} +export const someAction = actionCreatorFactory('SOME_ACTION').create(); + +// later when dispatched +someAction({ data: 'best practices' }); +``` + +```typescript +// declaring an action creator with a type string that has already been defined will throw +export const someAction = actionCreatorFactory('SOME_ACTION').create(); +export const theAction = actionCreatorFactory('SOME_ACTION').create(); // will throw +``` + +### noPayloadActionCreatorFactory + +Used when you don't need to supply a payload for your action. Will create an action creator with the following signature + +```typescript +{ type: string , (): {type: string; payload: undefined;} } +``` + +where the `type` string will be ensured to be unique. + +#### Example + +```typescript +export const noPayloadAction = noPayloadActionCreatorFactory('NO_PAYLOAD').create(); + +// later when dispatched +noPayloadAction(); +``` + +```typescript +// declaring an action creator with a type string that has already been defined will throw +export const noPayloadAction = noPayloadActionCreatorFactory('NO_PAYLOAD').create(); +export const noAction = noPayloadActionCreatorFactory('NO_PAYLOAD').create(); // will throw +``` + +### reducerFactory + +Fluent API used to create a reducer. (same as implementing the standard switch statement in Redux) + +#### Example + +```typescript +interface ExampleReducerState { + data: string[]; +} + +const intialState: ExampleReducerState = { data: [] }; + +export const someAction = actionCreatorFactory('SOME_ACTION').create(); +export const otherAction = actionCreatorFactory('Other_ACTION').create(); + +export const exampleReducer = reducerFactory(intialState) + // addMapper is the function that ties an action creator to a state change + .addMapper({ + // action creator to filter out which mapper to use + filter: someAction, + // mapper function where the state change occurs + mapper: (state, action) => ({ ...state, data: state.data.concat(action.payload) }), + }) + // a developer can just chain addMapper functions until reducer is done + .addMapper({ + filter: otherAction, + mapper: (state, action) => ({ ...state, data: action.payload }), + }) + .create(); // this will return the reducer +``` + +#### Typing limitations + +There is a challenge left with the mapper function that I can not solve with TypeScript. The signature of a mapper is + +```typescript +(state: State, action: ActionOf) => State; +``` + +If you would to return an object that is not of the state type like the following mapper + +```typescript +mapper: (state, action) => ({ nonExistingProperty: ''}), +``` + +Then you would receive the following compile error + +```shell +[ts] Property 'data' is missing in type '{ nonExistingProperty: string; }' but required in type 'ExampleReducerState'. [2741] +``` + +But if you return an object that is spreading state and add a non existing property type like the following mapper + +```typescript +mapper: (state, action) => ({ ...state, nonExistingProperty: ''}), +``` + +Then you would not receive any compile error. + +If you want to make sure that never happens you can just supply the State type to the mapper callback like the following mapper: + +```typescript +mapper: (state, action): ExampleReducerState => ({ ...state, nonExistingProperty: 'kalle' }), +``` + +Then you would receive the following compile error + +```shell +[ts] +Type '{ nonExistingProperty: string; data: string[]; }' is not assignable to type 'ExampleReducerState'. + Object literal may only specify known properties, and 'nonExistingProperty' does not exist in type 'ExampleReducerState'. [2322] +``` + +## New test functionality + +### reducerTester + +Fluent API that simplifies the testing of reducers + +#### Example + +```typescript +reducerTester() + .givenReducer(someReducer, initialState) + .whenActionIsDispatched(someAction('reducer tests')) + .thenStateShouldEqual({ ...initialState, data: 'reducer tests' }); +```