Skip to content

Commit

Permalink
Enable shopify/no-ancestor-directory-import eslint rule
Browse files Browse the repository at this point in the history
Importing from the component index leads to circular depdendencies that
cause issues when trying to optimize builds upstream. See this essay
https://github.com/Shopify/sewing-kit/blob/2bbf40fee4b6fc6e8a2cbbab1b743b47532b98eb/src/tools/webpack/tests/config/rules/javascript.test.ts#L215-L268

This commit adds tooling to help idenify cycles and stop them, so that
sewing-kit can remove its workarounds.

* Depdendency Cruiser (`yarn run depcruise-validate`) shall help
idenfity cycles.
* shopify/no-ancestor-directory-import is an eslint rule that bars
imports from parent indexes, which is the most common cause of cycles.

Fix current ancestort imports to import from direct files. This is not
enforced in tests because. test files will not be imported by anything
else so there is no danger of cylical imports.
  • Loading branch information
BPScott committed Nov 10, 2018
1 parent b26a9a6 commit 71f68ea
Show file tree
Hide file tree
Showing 53 changed files with 541 additions and 175 deletions.
129 changes: 129 additions & 0 deletions .dependency-cruiser.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
{
"forbidden": [
{
"name": "not-to-test",
"comment": "Don't allow dependencies from outside the test folder to test",
"severity": "error",
"from": {"pathNot": "(^(tests|spec))|\\.test\\.(ts|tsx)$"},
"to": {"path": "^(tests|spec)"}
},
{
"name": "not-to-spec",
"comment": "Don't allow dependencies to (typescript/ javascript/ coffeescript) spec files",
"severity": "error",
"from": {},
"to": {
"path": "\\.(test|spec)\\.(js|ts|ls|coffee|litcoffee|coffee\\.md)$"
}
},
{
"name": "no-circular",
"severity": "warn",
"comment": "Warn in case there's circular dependencies",
"from": {},
"to": {"circular": true}
},
{
"name": "no-orphans",
"severity": "info",
"comment": "Inform in case there's orphans hiding in the code base",
"from": {
"orphan": true,
"pathNot": "(^src/styles/polaris-tokens)|\\.d\\.ts$"
},
"to": {}
},
{
"name": "no-deprecated-core",
"comment": "Warn about dependencies on deprecated core modules.",
"severity": "warn",
"from": {},
"to": {
"dependencyTypes": ["core"],
"path": "^(punycode|domain|constants|sys|_linklist)$"
}
},
{
"name": "no-deprecated-npm",
"comment": "These npm modules are deprecated - find an alternative.",
"severity": "warn",
"from": {},
"to": {"dependencyTypes": ["deprecated"]}
},
{
"name": "not-to-unresolvable",
"comment": "Don't allow dependencies on modules dependency-cruiser can't resolve to files on disk (which probably means they don't exist)",
"severity": "error",
"from": {},
"to": {"couldNotResolve": true}
},
{
"name": "not-to-dev-dep",
"severity": "error",
"comment": "Don't allow dependencies from src/app/lib to a development only package",
"from": {
"path": "^(src|app|lib)",
"pathNot": "\\.(spec|test)\\.(js|ts|tsx|ls|coffee|litcoffee|coffee\\.md)$"
},
"to": {
"dependencyTypes": ["npm-dev"],
"pathNot": "^node_modules/(react|enzyme)"
}
},
{
"name": "no-non-package-json",
"severity": "error",
"comment": "Don't allow dependencies to packages not in package.json (except from within node_modules)",
"from": {"pathNot": "^node_modules"},
"to": {
"dependencyTypes": [
"unknown",
"undetermined",
"npm-no-pkg",
"npm-unknown"
]
}
},
{
"name": "optional-deps-used",
"severity": "info",
"comment": "nothing serious - but just check you have some serious try/ catches around the import/ requires of these",
"from": {},
"to": {"dependencyTypes": ["npm-optional"]}
},
{
"name": "peer-deps-used",
"comment": "Warn about the use of a peer dependency (peer dependencies are deprecated with the advent of npm 3 - and probably gone with version 4).",
"severity": "warn",
"from": {},
"to": {"dependencyTypes": ["npm-peer"], "pathNot": "^node_modules/react"}
},
{
"name": "no-duplicate-dep-types",
"comment": "Warn if a dependency you're actually using occurs in your package.json more than once (technically: has more than one dependency type)",
"severity": "warn",
"from": {},
"to": {
"moreThanOneDependencyType": true,
"pathNot": "^node_modules/react"
}
}
],
"options": {
"doNotFollow": "node_modules" /* pattern specifying which files not to follow further when encountered*/,
// "exclude" : "", /* pattern specifying which files to exclude (regular expression) */
// "moduleSystems": ["amd", "cjs", "es6", "tsd"],/* list of module systems to cruise */
// "prefix": "", /* prefix for links in html and svg output (e.g. https://github.com/you/yourrepo/blob/develop/) */
// "tsPreCompilationDeps": false, /* if true detect dependencies that only exist before typescript-to-javascript compilation */
// "preserveSymlinks": false, /* if true leave symlinks untouched, otherwise use the realpath */
"tsConfig": {
/* Typescript project file ('tsconfig.json') to use for (1) compilation and (2) resolution (e.g. with the paths property) */
"fileName": "./tsconfig.json" /* The typescript project file to use. The fileName is relative to dependency-cruiser's current working directory. When not provided defaults to './tsconfig.json'.*/
}
// "webpackConfig": { /* Webpack configuration to use to get resolve options from */
// "fileName": "./webpack.conf.js", /* The webpack conf file to use (typically something like 'webpack.conf.js'). The fileName is relative to dependency-cruiser's current working directory. When not provided defaults to './webpack.conf.js'. */
// "env": {}, /* Environment to pass if your config file returns a function */
// "args": {} /* Arguments to pass if your config file returns a function. E.g. {mode: 'production'} if you want to use webpack 4's 'mode' feature */
//}
}
}
7 changes: 7 additions & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"react/no-array-index-key": "off",
"shopify/jest/no-vague-titles": "off",
"shopify/jsx-no-complex-expressions": "off",
"shopify/no-ancestor-directory-import": "error",
"jsx-a11y/label-has-for": [
2,
{
Expand All @@ -52,6 +53,12 @@
"jsx-a11y/no-noninteractive-element-to-interactive-role": "off"
},
"overrides": [
{
"files": ["**/*.test.{ts,tsx}"],
"rules": {
"shopify/no-ancestor-directory-import": "off"
}
},
{
"files": ["examples/**/*.js"],
"rules": {
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"test:ci": "yarn test --coverage",
"check": "npm-run-all lint ts test",
"check:ci": "npm-run-all lint ts test:ci",
"depcruise-validate": "depcruise src --validate",
"clean": "rimraf build build-esnext esnext styles types docs 'build-intermediate' 'index.*' './src/styles/polaris-tokens' './tophat/build' 'styles.{css,scss}'",
"clean:build": "rimraf 'build/!(cache|coverage)' build-esnext esnext styles types docs 'build-intermediate' 'index.*' './src/styles/polaris-tokens' './tophat/build' 'styles.{css,scss}'",
"optimize": "node ./scripts/optimize.js",
Expand Down Expand Up @@ -122,6 +123,7 @@
"crypto": "^1.0.1",
"css-loader": "^0.28.11",
"cssnano": "^3.10.0",
"dependency-cruiser": "^4.6.0",
"enzyme": "^3.7.0",
"enzyme-adapter-react-16": "^1.6.0",
"express": "^4.16.3",
Expand Down
10 changes: 8 additions & 2 deletions src/components/AccountConnection/AccountConnection.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import * as React from 'react';
import {Avatar, buttonFrom, Card, Stack, TextStyle} from '..';
import SettingAction from '../SettingAction';

import {Action} from '../../types';
import Avatar from '../Avatar';
import {buttonFrom} from '../Button';
import Card from '../Card';
import Stack from '../Stack';
import TextStyle from '../TextStyle';
import SettingAction from '../SettingAction';

import * as styles from './AccountConnection.scss';

export interface Props {
Expand Down
2 changes: 1 addition & 1 deletion src/components/AppProvider/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Intl from './Intl';
import Link from './Link';
import StickyManager from './StickyManager';
import ScrollLockManager from './ScrollLockManager';
import {Props as AppProviderProps} from '.';
import {Props as AppProviderProps} from './AppProvider';

export const polarisAppProviderContextTypes: ValidationMap<any> = {
polaris: PropTypes.any,
Expand Down
7 changes: 4 additions & 3 deletions src/components/Autocomplete/Autocomplete.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import * as React from 'react';

import {ActionListItemDescriptor} from '../../types';
import {withAppProvider, WithAppProviderProps} from '../AppProvider';
import {PreferredPosition} from '../PositionedOverlay';
import {OptionDescriptor} from '../OptionList';
import {ActionListItemDescriptor} from '../../types';
import {TextFieldProps, Spinner} from '..';
import {ComboBox} from './components';
import Spinner from '../Spinner';
import {Props as TextFieldProps} from '../TextField';

import {ComboBox} from './components';
import * as styles from './Autocomplete.scss';

export interface Props {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as React from 'react';
import {autobind} from '@shopify/javascript-utilities/decorators';

import {TextField as BaseTextField, TextFieldProps} from '../../../../..';
import {contextTypes} from '../../../types';
import BaseTextField, {Props as TextFieldProps} from '../../../../../TextField';

export default class TextField extends React.PureComponent<
TextFieldProps,
Expand Down
4 changes: 3 additions & 1 deletion src/components/ButtonGroup/components/Item/Item.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import * as React from 'react';
import {autobind} from '@shopify/javascript-utilities/decorators';
import {classNames} from '@shopify/react-utilities/styles';
import {ButtonProps} from '../../..';

import {Props as ButtonProps} from '../../../Button';

import * as styles from '../../ButtonGroup.scss';

export interface Props {
Expand Down
18 changes: 12 additions & 6 deletions src/components/DropZone/components/FileUpload/FileUpload.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,26 @@
import * as React from 'react';
import capitalize from 'lodash/capitalize';
import {classNames} from '@shopify/react-utilities/styles';

import {WithContextTypes} from '../../../../types';
import compose from '../../../../utilities/react-compose';
import withRef from '../../../WithRef';

import {Link, Icon, Stack, Button, Caption, TextStyle} from '../../..';
import withContext from '../../../WithContext';
import {Consumer} from '../Context';
import {withAppProvider, WithAppProviderProps} from '../../../AppProvider';
import Link from '../../../Link';
import Icon from '../../../Icon';
import Stack from '../../../Stack';
import Button from '../../../Button';
import Caption from '../../../Caption';
import TextStyle from '../../../TextStyle';
import withContext from '../../../WithContext';
import withRef from '../../../WithRef';

import {DropZoneContext} from '../../types';
import IconDragDrop from '../../icons/drag-drop.svg';
import AssetFileUpload from '../../images/file-upload.svg';
import AssetImageUpload from '../../images/image-upload.svg';

import {DropZoneContext} from '../../types';
import {WithContextTypes} from '../../../../types';
import {Consumer} from '../Context';

import * as styles from './FileUpload.scss';

Expand Down
5 changes: 4 additions & 1 deletion src/components/EmptySearchResult/EmptySearchResult.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import * as React from 'react';

import {withAppProvider, WithAppProviderProps} from '../AppProvider';
import {DisplayText, TextStyle, Image, Stack} from '..';
import DisplayText from '../DisplayText';
import TextStyle from '../TextStyle';
import Image from '../Image';
import Stack from '../Stack';

import emptySearch from './illustrations/empty-search.svg';
import styles from './EmptySearchResult.scss';
Expand Down
3 changes: 1 addition & 2 deletions src/components/ExceptionList/ExceptionList.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import * as React from 'react';
import {classNames, variationName} from '@shopify/react-utilities/styles';

import Icon from '../Icon';
import Icon, {Props as IconProps} from '../Icon';
import Truncate from '../Truncate';
import {IconProps} from '..';

import * as styles from './ExceptionList.scss';

Expand Down
17 changes: 7 additions & 10 deletions src/components/Frame/Frame.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,13 @@ import {autobind} from '@shopify/javascript-utilities/decorators';
import {classNames} from '@shopify/react-utilities/styles';
import {CSSTransition} from 'react-transition-group';
import {navigationBarCollapsed} from '../../utilities/breakpoints';
import {
Button,
Icon,
EventListener,
ToastProps,
withAppProvider,
WithAppProviderProps,
Backdrop,
TrapFocus,
} from '..';
import Button from '../Button';
import Icon from '../Icon';
import EventListener from '../EventListener';
import {Props as ToastProps} from '../Toast';
import {withAppProvider, WithAppProviderProps} from '../AppProvider';
import Backdrop from '../Backdrop';
import TrapFocus from '../TrapFocus';
import {dataPolarisTopBar, layer, Duration} from '../shared';
import {setRootProperty} from '../../utilities/setRootProperty';
import {FrameContext, frameContextTypes} from '../types';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import * as React from 'react';

import {autobind} from '@shopify/javascript-utilities/decorators';
import {Button, Image, Stack, ContextualSaveBarProps} from '../../..';
import {withAppProvider, WithAppProviderProps} from '../../../AppProvider';

import {getWidth} from '../../../../utilities/getWidth';

import {withAppProvider, WithAppProviderProps} from '../../../AppProvider';
import Button from '../../../Button';
import {ContextualSaveBarProps} from '../../../ContextualSaveBar';
import Image from '../../../Image';
import Stack from '../../../Stack';

import {DiscardConfirmationModal} from './components';

import * as styles from './ContextualSaveBar.scss';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import * as React from 'react';

import {Modal, withAppProvider, WithAppProviderProps} from '../../../../..';
import {
withAppProvider,
WithAppProviderProps,
} from '../../../../../AppProvider';
import Modal from '../../../../../Modal';

export interface Props {
open: boolean;
Expand Down
7 changes: 5 additions & 2 deletions src/components/Frame/components/Toast/Toast.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import * as React from 'react';
import {classNames} from '@shopify/react-utilities';
import {KeypressListener, ToastProps, DEFAULT_TOAST_DURATION} from '../../..';
import Icon from '../../../Icon';

import {Key} from '../../../../types';

import Icon from '../../../Icon';
import KeypressListener from '../../../KeypressListener';
import {Props as ToastProps, DEFAULT_TOAST_DURATION} from '../../../Toast';

import * as styles from './Toast.scss';

export type Props = ToastProps;
Expand Down
6 changes: 4 additions & 2 deletions src/components/Frame/components/ToastManager/ToastManager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import * as React from 'react';
import {TransitionGroup, CSSTransition} from 'react-transition-group';
import {autobind} from '@shopify/javascript-utilities/decorators';
import {classNames} from '@shopify/react-utilities/styles';
import Toast from '../Toast';
import {Portal, EventListener, ToastProps} from '../../..';
import EventListener from '../../../EventListener';
import Portal from '../../../Portal';
import Toast, {Props as ToastProps} from '../Toast';

import * as styles from './ToastManager.scss';

export interface Props {
Expand Down
2 changes: 1 addition & 1 deletion src/components/Loading/Loading.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import {Loading as AppBridgeLoading} from '@shopify/app-bridge/actions';
import {FrameContext, frameContextTypes} from '../types';
import {withAppProvider, WithAppProviderProps} from '..';
import {withAppProvider, WithAppProviderProps} from '../AppProvider';

export interface Props {}
export type ComposedProps = Props & WithAppProviderProps;
Expand Down
Loading

0 comments on commit 71f68ea

Please sign in to comment.