Skip to content

Commit

Permalink
[Filters] Fix hitting max call depth when appliedFilters is undefined (
Browse files Browse the repository at this point in the history
…Shopify#10711)

### WHY are these changes introduced?

Currently `appliedFilters` can be `undefined`, except when it is when we
try to `setLocalPinnedFilters`, we'll do so forever because we fail the
check to decide whether to try set state or not every time, as
`!allAppliedFilterKeysInLocalPinnedFilters` will always be `true`.

To avoid this, we should only try to make the local pinned filters match
if we actually have any applied filters - and it's not `undefined`.

Found this while trying to bump to `v11.20.0` from `v11.19.0`.

This was introduced in:
* Shopify#10566

### WHAT is this pull request doing?

Checking `appliedFilters` is `defined` before trying to
`setLocalPinnedFilters`.

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

Example test that shows a bunch of re-renders
```tsx
import React from 'react';
import {render} from '@testing-library/react';
import {Filters, PolarisTestProvider} from '@shopify/polaris';

function Test() {
  return (
    <PolarisTestProvider>
      <Filters filters={[]} />
    </PolarisTestProvider>
  );
}

describe('Testing', () => {
  it('does thing', () => {
    render(<Test />);
  });
});

```

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}
```

</details>

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
  • Loading branch information
maael committed Sep 27, 2023
1 parent 9227b7c commit 62ca9ba
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/seven-apricots-provide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': patch
---

Fixed issue with setting local pinned filters in `Filters` when no `appliedFilters` were provided.
6 changes: 3 additions & 3 deletions polaris-react/src/components/Filters/Filters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,9 @@ export function Filters({
);

useEffect(() => {
const allAppliedFilterKeysInLocalPinnedFilters = appliedFilterKeys?.every(
(value) => localPinnedFilters.includes(value),
);
const allAppliedFilterKeysInLocalPinnedFilters =
!appliedFilterKeys ||
appliedFilterKeys.every((value) => localPinnedFilters.includes(value));

if (!allAppliedFilterKeysInLocalPinnedFilters) {
setLocalPinnedFilters((currentLocalPinnedFilters: string[]): string[] => {
Expand Down

0 comments on commit 62ca9ba

Please sign in to comment.