Update to latest sewing-kit #1344

BPScott · 2019-04-24T13:01:17Z

WHY are these changes introduced?

Keeping up to date, includes new linting rules mostly, meaning when eslint-plugin-shopify jumps to use the new typescript parser there will be less effort to update it and get onto a newer typescript version.

WHAT is this pull request doing?

Update a few dependencies that we want to keep in sync with sewing-kit

Use sewing-kit optimize to optimize svgs instead of maintaining our
own script which does the same thing.

An update to babel-plugin-shopify has changed the default value of "useBuiltIns". We now explicitly set "useBuildIns: 'entry'" in our babel.config.js to maintain the same behaviour as previously. Not doing this results in adding ~50kb extra polyfill stuff.

This doesn't affect polaris when used as part of sewing-kit because sk doesn't use the stand-alone build.

To Review

This PR is split into multiple commits, one of which is all the automatic fixes from running yarn run format which contains the majority of the code changes (apart from the yarn.lock file). It might be easier to look at the commits one-by-one rather than scrolling through a load of automatic fixes.

To Test

Ensure tests / linting / type checks pass
Ensure aria labels still appear as you'd expect in the components where I had to add translations

BPScott · 2019-05-03T13:07:24Z

.eslintrc

@@ -56,20 +59,24 @@
    {
      "files": ["**/*.test.{ts,tsx}"],
      "rules": {
+        "jest/no-truthy-falsy": "off",
+        "shopify/jsx-no-hardcoded-content": "off",


We want to check for hardcoded content to make sure we're using translated strings in our source, but we don't care about that in our tests and examples.

BPScott · 2019-05-03T13:15:53Z

.eslintrc

    "shopify/no-ancestor-directory-import": "error",
+    "shopify/react-prefer-private-members": "off",


Sometimes we do actually want pubic functions

I wonder if we should enable this and add an ignore whenever public is used, which is only in Banner i think?

There's 36 violations currently, 1/3 of which are false positives warning about examples written in JS when you can't even use private members (bug raised: Shopify/web-configs#110), and many of the others seem related to legacy context stuff - subscribe/unsubscribe stuff. I don't know enough about the code they touch to know what should be private or not.

I'd totally be open to reevaluating this in the future, but I don't want to touch them now :)

BPScott · 2019-05-03T13:16:04Z

.eslintrc

    "import/named": "off",
    "import/no-named-as-default": "off",
    "react/button-has-type": "off",
    "react/no-array-index-key": "off",
    "react/jsx-fragments": ["error", "element"],
    "shopify/jsx-no-complex-expressions": "off",
+    "shopify/jsx-prefer-fragment-wrappers": "off",


Sometimes we want empty wrapping divs

BPScott · 2019-05-03T13:16:28Z

.eslintrc

@@ -28,13 +27,16 @@
        "allowBlockStart": false
      }
    ],
+    "babel/no-unused-expressions": "off",


This doesn't seem to work in the way that I'd expect

BPScott · 2019-05-03T13:17:31Z

.eslintrc

@@ -56,20 +59,24 @@
    {
      "files": ["**/*.test.{ts,tsx}"],
      "rules": {
+        "jest/no-truthy-falsy": "off",


we use isTruthy() / isFalsy() A LOT and I don't want to go and refactor everything

AndrewMusgrave

Awesome stuff 😄 most of the comments are about the same, let me know what you think.

src/components/Frame/components/Toast/tests/Toast.test.tsx

src/components/KeypressListener/tests/KeypressListener.test.tsx

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

src/components/TextField/tests/TextField.test.tsx

AndrewMusgrave · 2019-05-03T22:53:49Z

src/components/TopBar/components/UserMenu/context/Modifier.tsx

@@ -22,7 +22,7 @@ class Modifier extends React.Component<ComposedProps, {}> {
    return null;
  }

-  state = {};
+  state: any = {};


We can probably 🔥 this and change line 14 to never

I've got no idea what this does. So I elected for not changing the code and just making typescript happy. It looks like this component still has state it's just defined in the passed setMobileUserMenuProps function.

Thinking a little deeper, matching this type with the second arg of React.Component<ComposedProps, {}> is a better plan. I've changed it to state: {}.

AndrewMusgrave · 2019-05-03T22:58:02Z

I requested translations and since the translations have already been released feel free to mark as async when you want to 🚢

danrosenthal · 2019-05-06T20:44:40Z

What's the status on this PR @BPScott?

BPScott · 2019-05-07T10:15:47Z

I've rebased this atop master, fixed additional issues (autofixing jest's toEqual to toStrictEqual) and addressed PR feedback. I've also updated the babel config to retain the old useBuiltIns behaviour, which means the bundle size is unaffected (previously it grew by a substantial amount).

I've responded to your comments and this is now read for re-review @AndrewMusgrave

AndrewMusgrave

After a squash you should be good to go 🎉 😄

AndrewMusgrave · 2019-05-07T22:21:10Z

.eslintrc

    "shopify/no-ancestor-directory-import": "error",
+    "shopify/react-prefer-private-members": "off",


I wonder if we should enable this and add an ignore whenever public is used, which is only in Banner i think?

AndrewMusgrave · 2019-05-07T22:25:46Z

src/components/Frame/components/Toast/tests/Toast.test.tsx


-      listenerMap.keyup({keyCode: Key.Escape});
+      afterEach(() => {
+        (document.addEventListener as jest.Mock).mockRestore();


I'm ok with this, however an alternative could have been

let removeEventListener: jest.SpyInstace; ... removeEventListener = jest.spyOn(...) ... removeEventListener.mockRestore

Update a few dependencies that we want to keep in sync with sewing-kit Use `sewing-kit optimize` to optimize svgs instead of maintaining our own script which does the same thing.

BPScott temporarily deployed to polaris-react-pr-1344 April 24, 2019 13:01 Inactive

BPScott mentioned this pull request Apr 24, 2019

Update developer dependencies #1327

Merged

BPScott changed the title ~~Update to latest sewing-kit~~ [WIP] Update to latest sewing-kit Apr 24, 2019

BPScott force-pushed the bump-sk branch from 4707009 to 411f378 Compare May 1, 2019 17:28

BPScott temporarily deployed to polaris-react-pr-1344 May 1, 2019 17:28 Inactive

BPScott force-pushed the bump-sk branch from 411f378 to ac429bc Compare May 3, 2019 11:40

BPScott temporarily deployed to polaris-react-pr-1344 May 3, 2019 11:40 Inactive

BPScott force-pushed the bump-sk branch from ac429bc to 9f586c0 Compare May 3, 2019 11:50

BPScott temporarily deployed to polaris-react-pr-1344 May 3, 2019 11:50 Inactive

BPScott force-pushed the bump-sk branch from 9f586c0 to 2edc9a6 Compare May 3, 2019 12:44

BPScott temporarily deployed to polaris-react-pr-1344 May 3, 2019 12:44 Inactive

BPScott temporarily deployed to polaris-react-pr-1344 May 3, 2019 12:48 Inactive

BPScott changed the title ~~[WIP] Update to latest sewing-kit~~ Update to latest sewing-kit May 3, 2019

BPScott requested a review from AndrewMusgrave May 3, 2019 13:06

BPScott commented May 3, 2019

View reviewed changes

AndrewMusgrave reviewed May 3, 2019

View reviewed changes

BPScott force-pushed the bump-sk branch from 40309c0 to f704fcb Compare May 6, 2019 21:26

BPScott temporarily deployed to polaris-react-pr-1344 May 6, 2019 21:26 Inactive

BPScott temporarily deployed to polaris-react-pr-1344 May 6, 2019 21:35 Inactive

BPScott temporarily deployed to polaris-react-pr-1344 May 7, 2019 09:56 Inactive

BPScott force-pushed the bump-sk branch from cb9e162 to 309c051 Compare May 7, 2019 12:43

BPScott deployed to polaris-react-pr-1344 May 7, 2019 12:44 View deployment

AndrewMusgrave approved these changes May 7, 2019

View reviewed changes

BPScott added 3 commits May 8, 2019 11:26

Update to latest sewing-kit

0edecb7

Update a few dependencies that we want to keep in sync with sewing-kit Use `sewing-kit optimize` to optimize svgs instead of maintaining our own script which does the same thing.

Fixes from running sk format

751a492

Manual linting fixes and fix hardcoded translations

248306f

BPScott force-pushed the bump-sk branch from 309c051 to 248306f Compare May 8, 2019 09:27

BPScott requested a deployment to polaris-react-pr-1344 May 8, 2019 09:28 Abandoned

BPScott merged commit 4a2a7fe into master May 8, 2019

BPScott deleted the bump-sk branch May 8, 2019 09:38

chloerice temporarily deployed to production May 8, 2019 19:57 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to latest sewing-kit #1344

Update to latest sewing-kit #1344

BPScott commented Apr 24, 2019 •

edited

Loading

BPScott May 3, 2019

BPScott May 3, 2019

AndrewMusgrave May 7, 2019

BPScott May 8, 2019 •

edited by alexandcote

Loading

BPScott May 3, 2019

BPScott May 3, 2019

BPScott May 3, 2019

AndrewMusgrave left a comment

AndrewMusgrave May 3, 2019

BPScott May 7, 2019

AndrewMusgrave commented May 3, 2019

danrosenthal commented May 6, 2019

BPScott commented May 7, 2019

AndrewMusgrave left a comment

AndrewMusgrave May 7, 2019

AndrewMusgrave May 7, 2019

		"shopify/no-ancestor-directory-import": "error",
		"shopify/react-prefer-private-members": "off",

Update to latest sewing-kit #1344

Update to latest sewing-kit #1344

Conversation

BPScott commented Apr 24, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

To Review

To Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPScott May 8, 2019 • edited by alexandcote Loading

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

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewMusgrave commented May 3, 2019

danrosenthal commented May 6, 2019

BPScott commented May 7, 2019

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPScott commented Apr 24, 2019 •

edited

Loading

BPScott May 8, 2019 •

edited by alexandcote

Loading