Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[test] Introduce prettier into CI pipeline #12564

Merged
merged 3 commits into from
Aug 18, 2018
Merged

[test] Introduce prettier into CI pipeline #12564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2696e1
[test] add prettier to CircleCI
eps1lon Aug 17, 2018
39f749f
[core] run prettier
eps1lon Aug 17, 2018
69e63ed
[test] use prettier:check instead of implicit run via eslint
eps1lon Aug 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@ jobs:
steps:
- checkout
- *restore_repo
- run:
name: Check if yarn prettier was run
command: |
# Files changed on this branch
CHANGED_FILES=$(git diff --name-only master...)
# Files that should have been formatted while working on this branch
FORMATTED_FILES=$(yarn --silent prettier:files | grep "$CHANGED_FILES")
Copy link
Member

@oliviertassinari oliviertassinari Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we run prettier twice here? Would it be faster to run it once and to execute git diff --exit-code?
Also, I think that we scope the new check to the TypeScript files :).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find always did pipe *.d.ts and *.tsx to the prettier script. eslint only processes jsx? files though as far as I know.

I was never really a fan of eslint-plugin-prettier. Mostly this comes down to personal preference but also some actual minor issues: eslint has no parser for ts files which makes eslint --fix with eslint-plugin-prettier useless for ts files. eslint-plugin-prettier allows prettier configuration with .eslintrc which is ignored by ide plugins.

prettier:files is just a helper method at the moment which lists files that should be formatted. It is not actually running prettier --write.

prettier --write should never be used in a CI environment. That's the hole purpose of --list-different. Combine --write with git diff while reducing I/O operations.

It was intended that the new check enforces a consistent code style across ts and js file. Just like the existing prettier scripts already formats tsx? files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I understood what you mean. Since we run prettier via eslint we would run it here again on all jsx? files. I removed eslint-plugin-prettier to removed this redundancy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint --fix

I never used that. I have never been satisfied with the output.

I was never really a fan of eslint-plugin-prettier

Most people configure their editor to run eslint. The advantage of this plugin is that you know up front that you need to run prettier, it's just like another eslint rule. At least, it's my workflow. I don't automatically run prettier on save. This data is interesting: https://npm-stat.com/charts.html?package=eslint-plugin-prettier&package=prettier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well prettier supports more than jsx? while eslint only runs on jsx?. Typescript support might explain some difference.

I didn't think about editor integration though and while I also don't use formatOnSave I do frequently press the format hotkey.

# if we run prettier unconditionally prettier will exit with non-zero
# because no file path was given
if [ $FORMATTED_FILES ]; then
echo "changes, let's check if they are formatted"
yarn prettier:ci "$FORMATTED_FILES"
else
echo "no changes"
fi
- run:
name: Lint
command: yarn lint
Expand Down
4 changes: 1 addition & 3 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module.exports = {
ecmaVersion: 7,
sourceType: 'module',
},
plugins: ['babel', 'import', 'jsx-a11y', 'mocha', 'flowtype', 'material-ui', 'prettier'],
plugins: ['babel', 'import', 'jsx-a11y', 'mocha', 'flowtype', 'material-ui'],
settings: {
'import/resolver': {
webpack: {
Expand Down Expand Up @@ -104,8 +104,6 @@ module.exports = {
'flowtype/type-id-match': 'off',
'flowtype/use-flow-type': 'error',

'prettier/prettier': 'error',

'jsx-a11y/label-has-associated-control': 'off',
'jsx-a11y/label-has-for': 'off',
'jsx-a11y/no-autofocus': 'off', // We are a library, people do what they want.
Expand Down
6 changes: 4 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@
"docs:deploy": "git push material-ui-docs master:latest",
"lint": "eslint . --cache && echo \"eslint: no lint errors\"",
"lint:fix": "eslint . --cache --fix && echo \"eslint: no lint errors\"",
"prettier": "find . -name \"*.js\" -o -name \"*.d.ts\" -o -name \"*.tsx\" | grep -v -f .eslintignore | xargs prettier --write",
"prettier": "yarn --silent prettier:files | xargs prettier --write",
"prettier:files": "find . -name \"*.js\" -o -name \"*.d.ts\" -o -name \"*.tsx\" | grep -v -f .eslintignore",
"prettier:check": "yarn --silent prettier:files | xargs prettier --list-different",
"prettier:ci": "prettier --list-different",
"size": "size-limit",
"size:why": "size-limit --why packages/material-ui/build/index.js",
"size:overhead:why": "size-limit --why ./test/size/overhead.js",
Expand Down Expand Up @@ -98,7 +101,6 @@
"eslint-plugin-jsx-a11y": "^6.0.3",
"eslint-plugin-material-ui": "file:packages/eslint-plugin-material-ui",
"eslint-plugin-mocha": "^5.0.0",
"eslint-plugin-prettier": "^2.3.1",
"eslint-plugin-react": "^7.4.0",
"eslint-plugin-spellcheck": "^0.0.10",
"fg-loadcss": "^2.0.1",
Expand Down
7 changes: 5 additions & 2 deletions packages/material-ui/src/ButtonBase/TouchRipple.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import * as React from 'react';
import { TransitionGroup } from 'react-transition-group';
import { StandardProps } from '..';

export type TouchRippleProps = StandardProps<TransitionGroup.TransitionGroupProps, TouchRippleClassKey> & {
export type TouchRippleProps = StandardProps<
TransitionGroup.TransitionGroupProps,
TouchRippleClassKey
> & {
center?: boolean;
}
};

export type TouchRippleClassKey =
| 'root'
Expand Down
7 changes: 3 additions & 4 deletions packages/material-ui/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ export type AnyComponent<P = any> =
| (new (props: P) => React.Component)
| ((props: P & { children?: React.ReactNode }) => React.ReactElement<P> | null);

export type PropsOf<C extends AnyComponent> =
C extends new (props: infer P) => React.Component ? P :
C extends (props: infer P) => React.ReactElement<any> | null ? P :
never;
export type PropsOf<C extends AnyComponent> = C extends new (props: infer P) => React.Component
? P
: C extends (props: infer P) => React.ReactElement<any> | null ? P : never;

/**
* All standard components exposed by `material-ui` are `StyledComponents` with
Expand Down
20 changes: 12 additions & 8 deletions packages/material-ui/src/styles/withStyles.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,14 @@ export interface WithStylesOptions<ClassKey extends string = string>

export type ClassNameMap<ClassKey extends string = string> = Record<ClassKey, string>;

export type WithStyles<T extends string | StyleRules | StyleRulesCallback = string, IncludeTheme extends boolean | undefined = undefined> =
(IncludeTheme extends true ? WithTheme : Partial<WithTheme>)
& {
classes: ClassNameMap<
T extends string
? T
: T extends StyleRulesCallback<infer K> ? K : T extends StyleRules<infer K> ? K : never
export type WithStyles<
T extends string | StyleRules | StyleRulesCallback = string,
IncludeTheme extends boolean | undefined = undefined
> = (IncludeTheme extends true ? WithTheme : Partial<WithTheme>) & {
classes: ClassNameMap<
T extends string
? T
: T extends StyleRulesCallback<infer K> ? K : T extends StyleRules<infer K> ? K : never
>;
};

Expand All @@ -53,7 +54,10 @@ export interface StyledComponentProps<ClassKey extends string = string> {
innerRef?: React.Ref<any> | React.RefObject<any>;
}

export default function withStyles<ClassKey extends string, Options extends WithStylesOptions<ClassKey>>(
export default function withStyles<
ClassKey extends string,
Options extends WithStylesOptions<ClassKey>
>(
style: StyleRulesCallback<ClassKey> | StyleRules<ClassKey>,
options?: Options,
): {
Expand Down
26 changes: 15 additions & 11 deletions packages/material-ui/test/typescript/styles.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,23 +178,27 @@ const AllTheComposition = withTheme()(
<AllTheComposition />;

{
const Foo = withTheme()(class extends React.Component<WithTheme> {
render() {
return null;
}
});
const Foo = withTheme()(
class extends React.Component<WithTheme> {
render() {
return null;
}
},
);

<Foo />
<Foo />;
}

declare const themed: boolean;
{
// Test that withTheme: true guarantees the presence of the theme
const Foo = withStyles({}, { withTheme: true })(class extends React.Component<WithTheme> {
render() {
return <div style={{ margin: this.props.theme.spacing.unit }} />;
}
});
const Foo = withStyles({}, { withTheme: true })(
class extends React.Component<WithTheme> {
render() {
return <div style={{ margin: this.props.theme.spacing.unit }} />;
}
},
);
<Foo />;

const Bar = withStyles({}, { withTheme: true })(({ theme }) => (
Expand Down
15 changes: 0 additions & 15 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4346,13 +4346,6 @@ eslint-plugin-mocha@^5.0.0:
dependencies:
ramda "^0.25.0"

eslint-plugin-prettier@^2.3.1:
version "2.6.2"
resolved "https://registry.yarnpkg.com/eslint-plugin-prettier/-/eslint-plugin-prettier-2.6.2.tgz#71998c60aedfa2141f7bfcbf9d1c459bf98b4fad"
dependencies:
fast-diff "^1.1.1"
jest-docblock "^21.0.0"

eslint-plugin-react@^7.4.0:
version "7.11.0"
resolved "https://registry.yarnpkg.com/eslint-plugin-react/-/eslint-plugin-react-7.11.0.tgz#b3124af974c4da978e62a57ea49a7bc26f11e76d"
Expand Down Expand Up @@ -4673,10 +4666,6 @@ fast-deep-equal@^2.0.1:
version "2.0.1"
resolved "https://registry.yarnpkg.com/fast-deep-equal/-/fast-deep-equal-2.0.1.tgz#7b05218ddf9667bf7f370bf7fdb2cb15fdd0aa49"

fast-diff@^1.1.1:
version "1.1.2"
resolved "https://registry.yarnpkg.com/fast-diff/-/fast-diff-1.1.2.tgz#4b62c42b8e03de3f848460b639079920695d0154"

fast-glob@^2.0.2:
version "2.2.2"
resolved "https://registry.yarnpkg.com/fast-glob/-/fast-glob-2.2.2.tgz#71723338ac9b4e0e2fff1d6748a2a13d5ed352bf"
Expand Down Expand Up @@ -6115,10 +6104,6 @@ jest-diff@^23.2.0:
jest-get-type "^22.1.0"
pretty-format "^23.2.0"

jest-docblock@^21.0.0:
version "21.2.0"
resolved "https://registry.yarnpkg.com/jest-docblock/-/jest-docblock-21.2.0.tgz#51529c3b30d5fd159da60c27ceedc195faf8d414"

jest-get-type@^22.1.0:
version "22.4.3"
resolved "https://registry.yarnpkg.com/jest-get-type/-/jest-get-type-22.4.3.tgz#e3a8504d8479342dd4420236b322869f18900ce4"
Expand Down