Update sewing-kit #532

BPScott · 2018-11-01T23:38:01Z

WHY are these changes introduced?

Keeping up to date
Removes the need for some overrides in our jest configuration
Improves our linting, by using an updated version of eslint-plugin-shopify
- fixes some cases where linting rules were mistakenly disabled thanks to the typescript and react configs having precedence fights. We thought these rules were always enabled but they weren't. Now they are.
- import linting rules now work within typescript files. This enforces the order of imports (node builtins, then node module, then parent imports, then siblings), and ensures paths are a simple as possible.

WHAT is this pull request doing?

Updates to the latest version of sewing-kit
Updates files to comply with linting rules.

Review this PR one commit at a time as about half the size of this pr comes from 7f835d3 which is the result of running yarn format rather than any manual changes.

How to 🎩

yarn lint and yarn test should both pass.

Change around the ordering of imports so sibling imports come before parent imports and see yarn lint complain

Prior to this version of eslint-plugin-shopify some rules were inadvertently disabled due to ordering of plugins causing prior plugins to be disabled. This has now been fixed so we can see errors for things that were problematic.

Fixes issues relating to import order and useless path segments

BPScott · 2018-11-02T00:07:23Z

.eslintrc

@@ -26,9 +28,12 @@
        "allowBlockStart": false
      }
    ],
+    "import/no-cycle": "off",


Initially disabled rules!

import/no-cycle shall be enabled in a follow-up PR as this one is already noisy enough as-is, and fixing it will require a lot of changes and there's potential for weirdness as I unpick the cycles.

import/no-named-as-default I'm not really sure what to do with. This currently has lots of errors and fixing it may require rethinking how we we write imports/exports. It doesn't help that I think the rule itself might be a bit buggy.

shopify/jsx-no-complex-expressions we probably want to keep disabled, This has 10 violations currently and in those cases we can't avoid it without some small rewrites that are more complex than I am comfortable putting in this PR that is mostly brainless changes.

All sounds good 👍 I think we might wanna create follow up issues for all of these so we don't forget about them.

BPScott · 2018-11-02T00:08:04Z

scripts/build.js

@@ -38,9 +38,6 @@ rm('-rf', resolvePath(root, 'types/src'));

 mv(resolvePath(intermediateBuild, 'src/*'), intermediateBuild);

-const srcReadme = resolvePath(root, './src/components/README.md');


These variables were never used

BPScott · 2018-11-02T00:09:21Z

scripts/pa11y.js

@@ -62,7 +61,7 @@ async function runPa11y() {
    ),
  );

-  urls.map((path) => {
+  urls.forEach((path) => {


This stops eslint complaining that there is no return value as in this case we don't care about the return value

BPScott · 2018-11-02T00:11:11Z

src/components/AppProvider/utils/index.tsx

@@ -139,6 +138,7 @@ export function withSticky() {
      | React.ComponentClass<OwnProps & WithAppProviderProps> & C
      | React.SFC<OwnProps & WithAppProviderProps> & C,
  ): any & C {
+    // eslint-disable-next-line shopify/react-initialize-state


I'm not really sure what the correct fix is here so I hand waved it away as this is deep dark typescript magic that I didn't want to mess with

I think there is an issue with the order of the generics here. <{}, OwnProps & WithAppProviderProps> would mean {} is the props and OwnProps & WithAppProviderProps is the state which is probably why you're getting this error. Try changing it to <OwnProps & WithAppProviderProps, never>.

I'm scared, and @AndrewMusgrave was poking around this area in https://github.com/Shopify/polaris-react-deprecated/pull/2274.

I think that PR might solve this, and then because we've got the eslint-comments plugin it'll remind us to delete the ignore line if it is not needed

BPScott · 2018-11-02T00:13:51Z

src/components/ColorPicker/ColorPicker.tsx

@@ -42,7 +42,6 @@ export default class ColorPicker extends React.PureComponent<Props, State> {
      return;
    }

-    // eslint-disable-next-line react/no-did-mount-set-state


calling setState in componentDidMount shouldn't happen that often but it is a legitimate use according to the react docs. See jsx-eslint/eslint-plugin-react#1754 for context.

As such this rule has been disabled globally

BPScott · 2018-11-02T00:15:57Z

src/components/Page/tests/Page.test.tsx

@@ -5,7 +5,9 @@ import {Page, DisplayText, Card} from 'components';
 import {noop} from '../../../utilities/other';
 import {LinkAction} from '../../../types';
 import {Header} from '../components';
+// eslint-disable-next-line shopify/strict-component-boundaries


I'm not 100% sure what the right fix for this is, I think it might be pulling these types up into a higher level, as they're referenced in some other places too. But that's a fix more complex than what I want to do in this PR

Makes sense and I agree if we need them outside of the component they should probably be available through components/Header. Maybe create a follow up issue?

kvendrik

💯

BPScott had a problem deploying to polaris-react-pr-532 November 1, 2018 23:38 Failure

BPScott added 4 commits November 1, 2018 16:58

Update to latest sewing-kit

660cb41

Manually fix several linting issues

da9344e

Prior to this version of eslint-plugin-shopify some rules were inadvertently disabled due to ordering of plugins causing prior plugins to be disabled. This has now been fixed so we can see errors for things that were problematic.

Autofix linting issues

4959568

Fixes issues relating to import order and useless path segments

Add eslint-comments plugin to indentify unused disable comments

bf6676c

BPScott force-pushed the update-sk branch from d6c6c4a to bf6676c Compare November 1, 2018 23:59

BPScott had a problem deploying to polaris-react-pr-532 November 1, 2018 23:59 Failure

BPScott commented Nov 2, 2018

View reviewed changes

BPScott requested review from kvendrik and tmlayton November 2, 2018 00:16

BPScott added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Nov 2, 2018

kvendrik approved these changes Nov 2, 2018

View reviewed changes

BPScott merged commit 278560d into master Nov 2, 2018

BPScott deleted the update-sk branch November 2, 2018 17:08

BPScott added a commit that referenced this pull request Nov 2, 2018

Fix lint error as a result of not rebasing #532 before merge

04577dc

tmlayton temporarily deployed to production November 8, 2018 06:35 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update sewing-kit #532

Update sewing-kit #532

BPScott commented Nov 1, 2018

BPScott Nov 2, 2018 •

edited

kvendrik Nov 2, 2018

BPScott Nov 2, 2018

BPScott Nov 2, 2018

BPScott Nov 2, 2018

kvendrik Nov 2, 2018

BPScott Nov 2, 2018

BPScott Nov 2, 2018 •

edited

BPScott Nov 2, 2018

kvendrik Nov 2, 2018

kvendrik left a comment

		@@ -38,9 +38,6 @@ rm('-rf', resolvePath(root, 'types/src'));

		mv(resolvePath(intermediateBuild, 'src/*'), intermediateBuild);

		const srcReadme = resolvePath(root, './src/components/README.md');

Update sewing-kit #532

Update sewing-kit #532

Conversation

BPScott commented Nov 1, 2018

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

BPScott Nov 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPScott Nov 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvendrik left a comment

Choose a reason for hiding this comment

BPScott Nov 2, 2018 •

edited

BPScott Nov 2, 2018 •

edited