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

[Filters] Fix hitting max call depth when appliedFilters is undefined #10711

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

maael
Copy link
Contributor

@maael maael commented Sep 27, 2023

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:

WHAT is this pull request doing?

Checking appliedFilters is defined before trying to setLocalPinnedFilters.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Example test that shows a bunch of re-renders

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 />);
  });
});
Copy-paste this code in playground/Playground.tsx:
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>
  );
}

🎩 checklist

@maael maael added the Bug Something is broken and not working as intended in the system. label Sep 27, 2023
@maael maael self-assigned this Sep 27, 2023
@maael maael force-pushed the fix/filter-max-call-depth branch 2 times, most recently from 6316805 to 4feb30f Compare September 27, 2023 14:17
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`.

This was introduced in:
* #10566
@maael maael changed the title [WIP][Filter] Fix hitting max call depth when appliedFilters is undefined [Filter] Fix hitting max call depth when appliedFilters is undefined Sep 27, 2023
@maael maael marked this pull request as ready for review September 27, 2023 16:41
@maael maael requested review from a team and mrcthms September 27, 2023 16:43
@maael maael changed the title [Filter] Fix hitting max call depth when appliedFilters is undefined [Filters] Fix hitting max call depth when appliedFilters is undefined Sep 27, 2023
@maael
Copy link
Contributor Author

maael commented Sep 27, 2023

@mrcthms 👋 hey there - thanks for the review. First time contributing to Polaris, do I just squash and merge? How does this end up eventually in the package, is that all automatic? Anything for me to be aware of?

@mrcthms
Copy link
Contributor

mrcthms commented Sep 27, 2023

@maael Yeah, you can squash and merge, and Polaris will automatically merge this into the next release branch. You should see a PR in the Polaris PR list called "Version Packages" and once the merge has completed, you'll see your change in that branch. To release a new version of polaris-react you would merge that PR (once you've checked with any other authors of PRs also contained within the release), and then bump to web.

It's worth noting that I'm going to be refactoring the Filters component quite a bit, and removing everything within the useEffect where your changes have been made. But that won't be ready for around a day or two, so this should still def go out if we're seeing issues with the current implementation!

@maael
Copy link
Contributor Author

maael commented Sep 27, 2023

Sounds good, thanks for the info!

I'll merge this then, and I'll leave it for another person to ship the Version Packages PR that this ends up in.

@maael maael merged commit 40d1672 into main Sep 27, 2023
17 checks passed
@maael maael deleted the fix/filter-max-call-depth branch September 27, 2023 17:04
This was referenced Sep 27, 2023
aaronccasanova added a commit that referenced this pull request Sep 27, 2023
aaronccasanova added a commit that referenced this pull request Sep 27, 2023
…ndefined" (#10734)

Reverts #10711

Temporarily reverting this PR so the [color
migration](#10576) can be on a
dedicated release.
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken and not working as intended in the system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants