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.

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

feat(eslint): add eslint in addition to tslint #75

Merged
merged 2 commits into from Nov 13, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -0,0 +1,29 @@
{
// tells eslint to use the TypeScript parser
"parser": "@typescript-eslint/parser",
// tell the TypeScript parser that we want to use JSX syntax
"parserOptions": {
"tsx": true,
"jsx": true,
"useJSXTextNode": true,
"project": "./tsconfig.json",
"tsconfigRootDir": "."
},
// we want to use the recommended rules provided from the typescript plugin
"extends": [
"plugin:@typescript-eslint/recommended",
"plugin:patternfly-react/recommended"
This conversation was marked as resolved by seanforyou23

This comment has been minimized.

Copy link
@mturley

mturley Nov 11, 2019

It might be worthwhile to review the eslint-plugin-patternfly-react recommended rules, we haven't touched those in a long time and they were written in PF3 days.

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 Nov 11, 2019

Author Member

Agree, I added it because it was available and I wanted to respect the effort put into those previous rules. No problem removing this or updating those rules to be whatever people think is best. I'll leave in for now and people can feel free to remove it if they don't agree with what the rulesets enforce.

Should we open an issue to update these rules given the latest PF4 efforts? Might be worth discussing this in more detail since it impacts quite a bit of upstream development.

],
// includes the typescript specific rules found here: https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin#supported-rules
"plugins": ["@typescript-eslint", "react-hooks", "eslint-plugin-react-hooks"],
"rules": {
"@typescript-eslint/explicit-function-return-type": "off",
"react-hooks/rules-of-hooks": "error",
"react-hooks/exhaustive-deps": "warn",
"@typescript-eslint/interface-name-prefix": "off",
"prettier/prettier": "off",
This conversation was marked as resolved by seanforyou23

This comment has been minimized.

Copy link
@mturley

mturley Nov 11, 2019

Starting with Prettier turned off sounds good to me, there are definitely people who won't want it. But I wonder if later on it would be useful to have a recommended Prettier configuration be part of the seed project, even if it's something we describe how to add in the README. For people who agree with its opinions, it's a nice tool to keep code style consistent and running it as part of eslint is a good way to enforce those rules. That's probably a debate for later though :)

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 Nov 11, 2019

Author Member

I'm with ya. Maybe, for now, we leave as an option that users can toggle on their own. If we can come to some consensus on what rules to apply I'm fine with turning it on for the masses - I just haven't so far because it seems like a pretty controversial topic for some 😉

The good thing is, this PR brings us one step closer to having support in this area, so if a user really wants it - should just be a few minutes of swizzling the config to make it happen.

"import/no-unresolved": "off",
"import/extensions": "off",
"react/prop-types": "off"
}
}
@@ -11,7 +11,9 @@
"build": "webpack --config webpack.prod.js",
"start": "webpack-dev-server --hot --color --progress --info=true --config webpack.dev.js",
"test": "jest",
"lint": "tslint -c ./tslint.json --project .",
"tslint": "tslint -c ./tslint.json --project .",
"eslint": "eslint --ext tsx ./src/",
This conversation was marked as resolved by seanforyou23

This comment has been minimized.

Copy link
@mturley

mturley Nov 11, 2019

I wonder if we should include the .js file extension here too, in case people are bringing in JS code that they haven't converted to TS yet or something... I'm trying to determine if there's a legitimate reason to have JS files around if you've started your project with the TS seed project. Maybe not.

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 Nov 11, 2019

Author Member

Absolutely agree, good catch! TypeScript is still new and we should ensure our tooling supports both JS and TS both as first-class citizens.

"lint": "yarn tslint && yarn eslint",
"format": "prettier --check --write ./src/**/*.{tsx,ts,js}",
"build:bundle-profile": "webpack --config webpack.prod.js --profile --json > stats.json",
"bundle-profile:analyze": "yarn build:bundle-profile && webpack-bundle-analyzer ./stats.json",
@@ -20,25 +22,30 @@
"build:storybook": "build-storybook"
},
"devDependencies": {
"@storybook/addon-actions": "^5.2.1",
"@storybook/addon-info": "^5.2.1",
"@storybook/addon-links": "^5.2.1",
"@storybook/addons": "^5.2.1",
"@storybook/react": "^5.2.1",
"@storybook/addon-actions": "^5.2.5",
"@storybook/addon-info": "^5.2.5",
"@storybook/addon-links": "^5.2.5",
"@storybook/addons": "^5.2.5",
"@storybook/react": "^5.2.5",
"@types/enzyme": "^3.10.3",
"@types/enzyme-adapter-react-16": "^1.0.5",
"@types/jest": "^24.0.17",
"@types/node": "^12.7.1",
"@types/react": "^16.8.25",
"@types/react-dom": "^16.8.5",
"@types/react-router-dom": "^4.3.4",
"@types/jest": "^24.0.21",
"@types/node": "^12.12.5",
"@types/react": "^16.9.11",
"@types/react-dom": "^16.9.3",
"@types/react-router-dom": "^5.1.2",
"@types/storybook__react": "^4.0.2",
"@types/victory": "^33.0.0",
"@types/webpack": "^4.32.1",
"@types/webpack": "^4.39.8",
"@typescript-eslint/eslint-plugin": "^2.6.0",
"@typescript-eslint/parser": "^2.6.0",
"css-loader": "^3.2.0",
"enzyme": "^3.7.0",
"enzyme-adapter-react-16": "^1.7.0",
"enzyme-to-json": "^3.4.0",
"enzyme-adapter-react-16": "^1.15.1",
"enzyme-to-json": "^3.4.3",
"eslint": "^6.6.0",
"eslint-plugin-patternfly-react": "^0.2.3",
"eslint-plugin-react-hooks": "^2.2.0",
"file-loader": "^4.2.0",
"html-webpack-plugin": "^3.2.0",
"imagemin": "^7.0.0",
@@ -48,34 +55,34 @@
"prettier": "^1.15.2",
"prop-types": "^15.6.1",
"raw-loader": "^3.1.0",
"react": "^16.8.4",
"react": "^16.11.0",
"react-axe": "^3.0.2",
"react-docgen-typescript-loader": "^3.2.1",
"react-dom": "^16.6.3",
"react-dom": "^16.11.0",
"regenerator-runtime": "^0.13.3",
"rimraf": "^3.0.0",
"style-loader": "^1.0.0",
"svg-url-loader": "^3.0.0",
"terser-webpack-plugin": "^2.1.0",
"terser-webpack-plugin": "^2.2.1",
"ts-jest": "^24.0.0",
"ts-loader": "^6.0.2",
"ts-loader": "^6.2.1",
"tsconfig-paths-webpack-plugin": "^3.2.0",
"tslint": "^5.13.1",
"tslint-config-prettier": "^1.18.0",
"tslint-eslint-rules": "^5.4.0",
"tslint-react": "^4.0.0",
"tslint-react-hooks": "^2.2.1",
"typescript": "^3.5.3",
"url-loader": "^2.1.0",
"webpack": "^4.39.1",
"webpack-bundle-analyzer": "^3.4.1",
"webpack-cli": "^3.3.6",
"webpack-dev-server": "^3.7.2",
"typescript": "^3.6.4",
"url-loader": "^2.2.0",
"webpack": "^4.41.2",
"webpack-bundle-analyzer": "^3.6.0",
"webpack-cli": "^3.3.10",
"webpack-dev-server": "^3.9.0",
"webpack-merge": "^4.1.4"
},
"dependencies": {
"@patternfly/react-core": "^3.77.2",
"@patternfly/react-icons": "^3.10.15",
"@patternfly/react-icons": "^3.14.18",
"@patternfly/react-styles": "^3.5.7",
"react-router-dom": "^5.0.1",
"react-router-last-location": "^2.0.1"
@@ -38,27 +38,26 @@ const AppLayout: React.FunctionComponent<IAppLayout> = ({children}) => {
logo="Patternfly"
logoProps={logoProps}
toolbar="Toolbar"
showNavToggle={true}
showNavToggle
isNavOpen={isNavOpen}
onNavToggle={isMobileView ? onNavToggleMobile : onNavToggle}
/>
);

const Navigation = (
<Nav id="nav-primary-simple">
<NavList id="nav-list-simple" variant={NavVariants.simple}>
{routes.map((route, idx) => {
return route.label && (
<Nav id="nav-primary-simple" theme="dark">
<NavList id="nav-list-simple" variant={NavVariants.default}>
{routes.map((route, idx) => route.label && (
<NavItem key={`${route.label}-${idx}`} id={`${route.label}-${idx}`}>
<NavLink exact={true} to={route.path} activeClassName="pf-m-current">{route.label}</NavLink>
<NavLink exact to={route.path} activeClassName="pf-m-current">{route.label}</NavLink>
</NavItem>
);
})}
))}
</NavList>
</Nav>
);
const Sidebar = (
<PageSidebar
theme="dark"
nav={Navigation}
isNavOpen={isMobileView ? isNavOpenMobile : isNavOpen} />
);
@@ -1,12 +1,10 @@
import * as React from 'react';
import { PageSection, Title } from '@patternfly/react-core';

const Dashboard: React.FunctionComponent<any> = (props) => {
return (
const Dashboard: React.FunctionComponent<{}> = () => (
This conversation was marked as resolved by seanforyou23

This comment has been minimized.

Copy link
@mturley

mturley Nov 11, 2019

This is the first time I've seen the <{}> syntax, do you know what the deal is with that? Is it just another way to write any?

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 Nov 11, 2019

Author Member

I believe this just signifies that the component doesn't take or require any props at all while supplying a type of any would allow any type of props to be passed.

<PageSection>
<Title size="lg">Dashboard Page Title</Title>
</PageSection>
);
}
)

export { Dashboard };
@@ -2,7 +2,9 @@ import * as React from 'react';
import { accessibleRouteChangeHandler } from '@app/utils/utils';

interface IDynamicImport {
// eslint-disable-next-line
This conversation was marked as resolved by seanforyou23

This comment has been minimized.

Copy link
@mturley

mturley Nov 11, 2019

It's totally cool to disable eslint where we need to, but it might be nice to only disable specific rule(s) so it's more clear why you're disabling it for that line. You can just put the rule names separated by commas after the disable directive, like // eslint-disable no-alert, no-console, I think it works both with and without -next-line.

load: () => Promise<any>;
// eslint-disable-next-line
children: any;
focusContentAfterMount: boolean;
}
@@ -2,13 +2,11 @@ import * as React from 'react';
import { NavLink } from 'react-router-dom';
import { Alert, PageSection } from '@patternfly/react-core';

const NotFound: React.FunctionComponent = () => {
return (
const NotFound: React.FunctionComponent = () => (
<PageSection>
<Alert variant="danger" title="404! This view hasn't been created yet." /><br />
<NavLink to="/" className="pf-c-nav__link">Take me home</NavLink>
</PageSection>
);
}
)

export { NotFound };
@@ -15,16 +15,15 @@ export interface ISupportProps {
sampleProp?: string;
}

const Support: React.FunctionComponent<ISupportProps> = (props) => {
return (
const Support: React.FunctionComponent<ISupportProps> = () => (
<PageSection>
<EmptyState variant={EmptyStateVariant.full}>
<EmptyStateIcon icon={CubesIcon} />
<Title headingLevel="h5" size="lg">
Empty State (Stub Support Module)
</Title>
<EmptyStateBody>
This represents an the empty state pattern in Patternfly 4. Hopefully it's simple enough to use but flexible
This represents an the empty state pattern in Patternfly 4. Hopefully it&apos;s simple enough to use but flexible
enough to meet a variety of needs.
</EmptyStateBody>
<Button variant="primary">Primary Action</Button>
@@ -38,7 +37,6 @@ const Support: React.FunctionComponent<ISupportProps> = (props) => {
</EmptyStateSecondaryActions>
</EmptyState>
</PageSection>
);
}
)

export { Support };
@@ -18,8 +18,9 @@ describe('App tests', () => {
it('should hide the sidebar when clicking the nav-toggle button', () => {
const wrapper = mount(<App />);
const button = wrapper.find('#nav-toggle').hostNodes();
expect(wrapper.find('#page-sidebar').hasClass('pf-m-expanded'));
expect(wrapper.find('#page-sidebar').hasClass('pf-m-expanded')).toBeTruthy();
button.simulate('click');
expect(wrapper.find('#page-sidebar').hasClass('pf-m-collapsed'));
expect(wrapper.find('#page-sidebar').hasClass('pf-m-collapsed')).toBeTruthy();
expect(wrapper.find('#page-sidebar').hasClass('pf-m-expanded')).toBeFalsy();
});
});
@@ -5,14 +5,12 @@ import { AppLayout } from '@app/AppLayout/AppLayout';
import { AppRoutes } from '@app/routes';
import '@app/app.css';

const App: React.FunctionComponent = () => {
return (
const App: React.FunctionComponent = () => (
<Router>
<AppLayout>
<AppRoutes />
</AppLayout>
</Router>
);
};

export { App };
@@ -10,16 +10,14 @@ import { LastLocationProvider, useLastLocation } from 'react-router-last-locatio

let routeFocusTimer: number;

const getSupportModuleAsync = () => {
return () => import(/* webpackChunkName: 'support' */ '@app/Support/Support');
};
const getSupportModuleAsync = () => () => import(/* webpackChunkName: 'support' */ '@app/Support/Support');

const Support = (routeProps: RouteComponentProps) => {
const lastNavigation = useLastLocation();
return (
<DynamicImport load={getSupportModuleAsync()} focusContentAfterMount={lastNavigation !== null}>
{(Component: any) => {
let loadedComponent: any;
{(Component: any) => { // eslint-disable-line
let loadedComponent: any; // eslint-disable-line
if (Component === null) {
loadedComponent = (
<PageSection aria-label="Loading Content Container">
@@ -39,8 +37,7 @@ const Support = (routeProps: RouteComponentProps) => {

export interface IAppRoute {
label?: string;
component: React.ComponentType<RouteComponentProps<any>> | React.ComponentType<any>;
icon?: any;
component: React.ComponentType<RouteComponentProps<any>> | React.ComponentType<any>; // eslint-disable-line
exact?: boolean;
path: string;
title: string;
@@ -51,15 +48,13 @@ const routes: IAppRoute[] = [
{
component: Dashboard,
exact: true,
icon: null,
label: 'Dashboard',
path: '/',
title: 'Main Dashboard Title'
},
{
component: Support,
exact: true,
icon: null,
isAsync: true,
label: 'Support',
path: '/support',
@@ -108,18 +103,17 @@ const PageNotFound = ({ title }: { title: string }) => {
const AppRoutes = () => (
<LastLocationProvider>
<Switch>
{routes.map(({ path, exact, component, title, isAsync, icon }, idx) => (
{routes.map(({ path, exact, component, title, isAsync }, idx) => (
<RouteWithTitleUpdates
path={path}
exact={exact}
component={component}
key={idx}
icon={icon}
title={title}
isAsync={isAsync}
/>
))}
<PageNotFound title={'404 Page Not Found'} />
<PageNotFound title="404 Page Not Found" />
</Switch>
</LastLocationProvider>
);
@@ -4,7 +4,7 @@ import { App } from '@app/index';

if (process.env.NODE_ENV !== "production") {
// tslint:disable-next-line
const axe = require("react-axe");
const axe = require("react-axe"); // eslint-disable-line
axe(React, ReactDOM, 1000);
}

@@ -13,6 +13,7 @@
},
"rules": {
"ordered-imports": [false],
"react-hooks-nesting": "error"
"react-hooks-nesting": "error",
"jsx-boolean-value": ["never"]
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.