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

Update to latest sewing-kit #1344

Merged
merged 3 commits into from
May 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": [
"plugin:shopify/typescript",
"plugin:shopify/react",
"plugin:shopify/eslint-comments",
"plugin:shopify/jest",
"plugin:shopify/node",
"plugin:shopify/polaris",
Expand All @@ -28,13 +27,16 @@
"allowBlockStart": false
}
],
"babel/no-unused-expressions": "off",
Copy link
Member Author

Choose a reason for hiding this comment

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

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

"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",
Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes we want empty wrapping divs

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

Choose a reason for hiding this comment

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

Sometimes we do actually want pubic functions

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@BPScott BPScott May 8, 2019

Choose a reason for hiding this comment

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

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 :)

"jsx-a11y/label-has-for": [
2,
{
Expand All @@ -46,6 +48,7 @@
],
"jsx-a11y/no-autofocus": "off",
"jsx-a11y/anchor-has-content": "off",
"jsx-a11y/control-has-associated-label": "off",
"jsx-a11y/role-supports-aria-props": "off",
"jsx-a11y/mouse-events-have-key-events": "off",
"jsx-a11y/click-events-have-key-events": "off",
Expand All @@ -56,20 +59,24 @@
{
"files": ["**/*.test.{ts,tsx}"],
"rules": {
"jest/no-truthy-falsy": "off",
Copy link
Member Author

Choose a reason for hiding this comment

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

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

"shopify/jsx-no-hardcoded-content": "off",
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"shopify/no-ancestor-directory-import": "off"
}
},
{
"files": ["examples/**/*.js"],
"rules": {
"import/no-extraneous-dependencies": "off",
"import/no-unresolved": "off"
"import/no-unresolved": "off",
"shopify/jsx-no-hardcoded-content": "off"
}
},
{
"files": ["playground/Playground.tsx"],
"rules": {
"react/prefer-stateless-function": "off",
"shopify/jsx-no-hardcoded-content": "off",
"shopify/react-initialize-state": "off"
}
}
Expand Down
3 changes: 2 additions & 1 deletion UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f
- Update most `devDependencies` ([#1327](https://github.com/Shopify/polaris-react/pull/1327))
- Bump `@shopify/react-utilites` to remove a transitive dependency on `core-js` ([#1343](https://github.com/Shopify/polaris-react/pull/1343))
- Updated App Bridge to version 1.3.0 ([#1349](https://github.com/Shopify/polaris-react/pull/1349))
- Updated TypeScript to 3.2.4 ([#1388](https://github.com/Shopify/polaris-react/pull/1388))
- Updated typescript to 3.2.4 ([#1388](https://github.com/Shopify/polaris-react/pull/1388))
- Updated sewing-kit to 0.83.1 and babel-preset-shopify to ^18.1.0 ([#1344](https://github.com/Shopify/polaris-react/pull/1344))

### Code quality

Expand Down
2 changes: 1 addition & 1 deletion babel.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module.exports = function(api) {
});

const runtimePreset = isWeb
? ['babel-preset-shopify/web', {modules: false}]
? ['babel-preset-shopify/web', {modules: false, useBuiltIns: 'entry'}]
: ['babel-preset-shopify/node', {modules: 'commonjs'}];

// babel-preset-shopify/react only uses HMR if hot is true and the env is
Expand Down
12 changes: 5 additions & 7 deletions examples/browserify/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class App extends Component {
{label: 'I consent to receiving emails', value: 'false2'},
];

const accountMarkup = this.state.connected
? this.disconnectAccountMarkup()
: this.connectAccountMarkup();

return (
<AppProvider>
<Page
Expand All @@ -58,7 +62,7 @@ class App extends Component {
</SettingToggle>
</Layout.AnnotatedSection>

{this.renderAccount()}
{accountMarkup}

<Layout.AnnotatedSection
title="Form"
Expand Down Expand Up @@ -173,12 +177,6 @@ class App extends Component {
</Layout.AnnotatedSection>
);
}

renderAccount() {
return this.state.connected
? this.disconnectAccountMarkup()
: this.connectAccountMarkup();
}
}

export default App;
12 changes: 5 additions & 7 deletions examples/create-react-app/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ class App extends Component {
{label: 'I consent to receiving emails', value: 'false2'},
];

const accountMarkup = this.state.connected
? this.disconnectAccountMarkup()
: this.connectAccountMarkup();

return (
<Page
title="Polaris"
Expand All @@ -59,7 +63,7 @@ class App extends Component {
</SettingToggle>
</Layout.AnnotatedSection>

{this.renderAccount()}
{accountMarkup}

<Layout.AnnotatedSection
title="Form"
Expand Down Expand Up @@ -173,12 +177,6 @@ class App extends Component {
</Layout.AnnotatedSection>
);
}

renderAccount() {
return this.state.connected
? this.disconnectAccountMarkup()
: this.connectAccountMarkup();
}
}

export default App;
12 changes: 5 additions & 7 deletions examples/webpack/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class App extends Component {
{label: 'I consent to receiving emails', value: 'false2'},
];

const accountMarkup = this.state.connected
? this.disconnectAccountMarkup()
: this.connectAccountMarkup();

return (
<AppProvider>
<Page
Expand All @@ -58,7 +62,7 @@ class App extends Component {
</SettingToggle>
</Layout.AnnotatedSection>

{this.renderAccount()}
{accountMarkup}

<Layout.AnnotatedSection
title="Form"
Expand Down Expand Up @@ -173,12 +177,6 @@ class App extends Component {
</Layout.AnnotatedSection>
);
}

renderAccount() {
return this.state.connected
? this.disconnectAccountMarkup()
: this.connectAccountMarkup();
}
}

export default App;
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"check:ci": "npm-run-all lint ts test:ci",
"clean": "rimraf build esnext styles types docs \"build-intermediate\" \"index.*\" \"./src/styles/polaris-tokens\" \"styles.{css,scss}\"",
"clean:build": "rimraf \"build/!(cache|coverage|storybook)\" esnext styles types docs \"build-intermediate\" \"index.*\" \"./src/styles/polaris-tokens\" \"styles.{css,scss}\"",
"optimize": "node ./scripts/optimize.js",
"optimize": "sewing-kit optimize",
"prebuild": "npm-run-all clean:build optimize copy-polaris-tokens",
"build": "node ./scripts/build.js",
"prebuild-consumer": "yarn run build",
Expand Down Expand Up @@ -91,11 +91,11 @@
"ios >= 9"
],
"devDependencies": {
"@babel/core": "^7.1.6",
"@babel/core": "^7.4.3",
"@percy/storybook": "^3.0.1",
"@shopify/jest-dom-mocks": "^2.1.1",
"@shopify/js-uploader": "github:shopify/js-uploader",
"@shopify/sewing-kit": "0.69.0",
"@shopify/sewing-kit": "0.83.1",
"@storybook/addon-a11y": "^5.0.6",
"@storybook/addon-actions": "^5.0.6",
"@storybook/addon-backgrounds": "^5.0.6",
Expand All @@ -112,7 +112,7 @@
"aws-sdk": "^2.58.0",
"babel-core": "7.0.0-bridge.0",
"babel-loader": "^8.0.5",
"babel-preset-shopify": "^17.0.1",
"babel-preset-shopify": "^18.1.1",
"chalk": "^2.4.2",
"change-case": "^3.1.0",
"codecov": "^3.3.0",
Expand Down
37 changes: 0 additions & 37 deletions scripts/optimize.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ describe('translate()', () => {
it('throws an error for a missing replacement', () => {
expect(() =>
translateWithReplacements('foo {next}', {notNext: 'bar'}),
).toThrow();
).toThrow(
`No replacement found for key 'next'. The following replacements were passed: 'notNext'`,
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ describe('createAppProviderContext()', () => {
},
});

expect(Intl).toBeCalledWith(i18n);
expect(Link).toBeCalledWith(CustomLinkComponent);
expect(Intl).toHaveBeenCalledWith(i18n);
expect(Link).toHaveBeenCalledWith(CustomLinkComponent);
});

it('adds an app bridge hook to set clientInterface data', () => {
Expand All @@ -130,7 +130,7 @@ describe('createAppProviderContext()', () => {
const next = jest.fn((args) => args);
const baseAction = {type: 'actionType'};

expect(setClientInterfaceHook.call({}, next)(baseAction)).toEqual({
expect(setClientInterfaceHook.call({}, next)(baseAction)).toStrictEqual({
type: 'actionType',
clientInterface: {
name: '@shopify/polaris',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ describe('createPolarisContext()', () => {
},
};

expect(contextOne).toEqual(mockContext);
expect(contextTwo).toEqual(mockContext);
expect(contextOne).toStrictEqual(mockContext);
expect(contextTwo).toStrictEqual(mockContext);
});

it('returns the right context with only app provider context being provided', () => {
Expand Down Expand Up @@ -140,7 +140,7 @@ describe('createPolarisContext()', () => {
},
};

expect(context).toEqual(mockContext);
expect(context).toStrictEqual(mockContext);
});

it('returns the right context with only theme provider context being provided', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ describe('<ComboBox/>', () => {
);

comboBox.simulate('click');
expect(comboBox.find(OptionList).prop('selected')).toEqual([
expect(comboBox.find(OptionList).prop('selected')).toStrictEqual([
'cheese_pizza',
]);
});
Expand Down
10 changes: 5 additions & 5 deletions src/components/Autocomplete/tests/Autocomplete.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,22 +72,22 @@ describe('<Autocomplete/>', () => {

expect(autocomplete.find(ComboBox).prop('id')).toBe('Autocomplete-ID');
expect(autocomplete.find(ComboBox).prop('options')).toBe(options);
expect(autocomplete.find(ComboBox).prop('selected')).toEqual([
expect(autocomplete.find(ComboBox).prop('selected')).toStrictEqual([
'cheese_pizza',
]);
expect(autocomplete.find(ComboBox).prop('textField')).toEqual(
expect(autocomplete.find(ComboBox).prop('textField')).toStrictEqual(
renderTextField(),
);
expect(autocomplete.find(ComboBox).prop('preferredPosition')).toBe(
'mostSpace',
);
expect(autocomplete.find(ComboBox).prop('listTitle')).toBe('List title');
expect(autocomplete.find(ComboBox).prop('allowMultiple')).toBe(true);
expect(autocomplete.find(ComboBox).prop('actionsBefore')).toEqual(
expect(autocomplete.find(ComboBox).prop('actionsBefore')).toStrictEqual(
actionBefore,
);
expect(autocomplete.find(ComboBox).prop('onSelect')).toBe(handleOnSelect);
expect(autocomplete.find(ComboBox).prop('emptyState')).toEqual(
expect(autocomplete.find(ComboBox).prop('emptyState')).toStrictEqual(
<EmptyState />,
);
});
Expand All @@ -104,7 +104,7 @@ describe('<Autocomplete/>', () => {
loading
/>,
);
expect(autocomplete.find(ComboBox).prop('options')).toEqual([]);
expect(autocomplete.find(ComboBox).prop('options')).toStrictEqual([]);
expect(autocomplete.find(ComboBox).prop('contentAfter')).not.toBeNull();
});
});
Expand Down
2 changes: 1 addition & 1 deletion src/components/CalloutCard/tests/CalloutCard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,6 @@ describe('<CalloutCard />', () => {
.first()
.simulate('click');

expect(spy).toBeCalled();
expect(spy).toHaveBeenCalled();
});
});
5 changes: 4 additions & 1 deletion src/components/Card/components/Header/tests/Header.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ describe('<Header />', () => {

it('renders buttons for each action', () => {
mountWithAppProvider(<Header actions={mockActions} />);
expect(buttonsFromMock).toBeCalledWith(mockActions, expect.anything());
expect(buttonsFromMock).toHaveBeenCalledWith(
mockActions,
expect.anything(),
);
});

it('does not render a button group when not defined', () => {
Expand Down
4 changes: 2 additions & 2 deletions src/components/Card/components/Section/tests/Section.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('<Card.Section />', () => {
const card = mountWithAppProvider(<Section title={titleMarkup} />);
const headerMarkup = card.find('h2');

expect(headerMarkup.text().includes(titleString)).toBe(true);
expect(headerMarkup.text()).toContain(titleString);
expect(headerMarkup.find('Badge').text()).toBe(badgeString);
});

Expand All @@ -28,6 +28,6 @@ describe('<Card.Section />', () => {
const headerMarkup = card.find(Subheading);

expect(headerMarkup.exists()).toBeTruthy();
expect(headerMarkup.text()).toEqual(titleString);
expect(headerMarkup.text()).toStrictEqual(titleString);
});
});
2 changes: 1 addition & 1 deletion src/components/Card/tests/Card.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('<Card />', () => {
const card = mountWithAppProvider(<Card title={titleMarkup} />);
const headerMarkup = card.find('h2');

expect(headerMarkup.text().includes(titleString)).toBe(true);
expect(headerMarkup.text()).toContain(titleString);
expect(headerMarkup.find('Badge').text()).toBe(badgeString);
});

Expand Down
Loading