From 854644f46cdc10387ef27399bbde7a61f835e9bf Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Mon, 18 Mar 2019 10:39:14 +0100 Subject: [PATCH 1/6] Add more patterns to no-only-test task --- scripts/grunt/default_task.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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); } }); }); From dcc5373e770035035889d55c0a543eea59ed26c8 Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Mon, 18 Mar 2019 10:40:04 +0100 Subject: [PATCH 2/6] Remove .only function --- .../src/components/ThresholdsEditor/ThresholdsEditor.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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' }, From 2b9cf1132f987ee3f1db9a606d5ec7fc09f471bb Mon Sep 17 00:00:00 2001 From: Oleg Gaidarenko Date: Mon, 18 Mar 2019 13:31:57 +0100 Subject: [PATCH 3/6] Use ora#fail instead of console.log Since with ora#fail you can stderr it instead of using the stdout, and it's a bit nicer since it will show that cross sign :) --- scripts/cli/utils/useSpinner.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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); } From f3b9ce317e793900f23dd27055a545f4aafbee17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Fri, 15 Mar 2019 11:51:13 +0100 Subject: [PATCH 4/6] docs: intial draft for frontend review doc --- style_guides/frontend-review-checklist.md | 67 +++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 style_guides/frontend-review-checklist.md diff --git a/style_guides/frontend-review-checklist.md b/style_guides/frontend-review-checklist.md new file mode 100644 index 00000000000..39c1dea8ee4 --- /dev/null +++ b/style_guides/frontend-review-checklist.md @@ -0,0 +1,67 @@ +# Frontend 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. +- [ ] The pull request does not increase the number of `implicit any` errors. +- [ ] The pull request does not contain uses of `any` or `{}` that are unexplainable. +- [ ] 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. + +### 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. + +### 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. +- [ ] The pull request uses `reducerTester` to test reducers. +- [ ] The pull request does not contain code that access reducers state slice directly, instead the code uses state selectors to access state. + +## Common bad practices + +### 1. Missing Props/State type + +- 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 = { ... } + ``` From f251345b6804d3dad328fa60008e81d94323db16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Mon, 18 Mar 2019 07:55:44 +0100 Subject: [PATCH 5/6] docs: moved examples to frontend.md --- style_guides/frontend-review-checklist.md | 34 ------------ style_guides/frontend.md | 65 ++++++++++++++++------- 2 files changed, 46 insertions(+), 53 deletions(-) diff --git a/style_guides/frontend-review-checklist.md b/style_guides/frontend-review-checklist.md index 39c1dea8ee4..139c963b42d 100644 --- a/style_guides/frontend-review-checklist.md +++ b/style_guides/frontend-review-checklist.md @@ -31,37 +31,3 @@ - [ ] The pull request uses helpers `actionCreatorFactory` and `reducerFactory` instead of traditional `switch statement` reducers in Redux. - [ ] The pull request uses `reducerTester` to test reducers. - [ ] The pull request does not contain code that access reducers state slice directly, instead the code uses state selectors to access state. - -## Common bad practices - -### 1. Missing Props/State type - -- 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/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 = { ... } +``` From ed1b00190479a5053ed3e7c14d18fb5d2f5479b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Mon, 18 Mar 2019 12:03:34 +0100 Subject: [PATCH 6/6] docs: renamed file and added redux framework file --- ...st.md => pull-request-review-checklist.md} | 17 +- style_guides/redux.md | 158 ++++++++++++++++++ 2 files changed, 168 insertions(+), 7 deletions(-) rename style_guides/{frontend-review-checklist.md => pull-request-review-checklist.md} (82%) create mode 100644 style_guides/redux.md diff --git a/style_guides/frontend-review-checklist.md b/style_guides/pull-request-review-checklist.md similarity index 82% rename from style_guides/frontend-review-checklist.md rename to style_guides/pull-request-review-checklist.md index 139c963b42d..2fd017386ea 100644 --- a/style_guides/frontend-review-checklist.md +++ b/style_guides/pull-request-review-checklist.md @@ -1,4 +1,4 @@ -# Frontend Review Checklist +# Pull Request Review Checklist ## High level checks @@ -15,19 +15,22 @@ - [ ] 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. -- [ ] The pull request does not increase the number of `implicit any` errors. -- [ ] The pull request does not contain uses of `any` or `{}` that are unexplainable. -- [ ] 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. ### 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. -- [ ] The pull request uses `reducerTester` to test reducers. +- [ ] 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' }); +```