Remove defaultProps usages for functional components

[undeletable]

What does this PR do?

Starting from React 19, defaultProps for functional components is deprecated. Starting from React 18.3, the corresponding error is thrown for a functional component with defaultProps defined.

Changes in this PR:

• replace defaultProps for functional components with default function parameters values;
• use attrs method of styled-components to pass theme to styled components which might be outside of theme context (required per remove defaultProps from Grommet #6741 (comment));
• add custom hook to obtain theme from context with fallback to default value;
• configure eslint rule to not require defaultProps for functional components;
• update snapshots.

Where should the reviewer start?

No specific place to start.

What testing has been done on this PR?

• manual testing on storybook and real project using grommet;
• tests run result:

Test Suites: 97 passed, 97 total
Tests:       1548 passed, 1548 total
Snapshots:   1777 passed, 1777 total

How should this be manually tested?

Use React 18.3+ in an app. Check that styles are applied correctly and no errors about defaultProps are thrown into console.

Do Jest tests follow these best practices?

• screen is used for querying.
• The correct query is used. (Refer to this list of queries)
• asFragment() is used for snapshot testing.

Any background context you want to provide?

React 19 upgrade guide

What are the relevant issues?

Closes #6741

Screenshots (if appropriate)

N/A.

Do the grommet docs need to be updated?

Just the internal ones (if any).

Should this PR be mentioned in the release notes?

Yes.

Is this change backwards compatible or is it a breaking change?

Changes are backwards compatible.

[britt6612]

@undeletable Thank you for your contribution! We are in the process of looking at this just wanted to let you know!

[jcfilben]

Thanks for working on this!

In the changes I saw that you are using the attrs method of styled-components to pass the theme. In the PR notes you referenced this comment #6741 (comment) as the reason for those changes. I think this comment was misinterpreting the original intent of removing defaultProps.

There is some confusing naming where we have defaultProps in the react sense and then we have defaultProps in Grommet that contains the base theme. I think #6741 (comment), misunderstood the issue and was thinking that we would remove the defaultProps that has the Grommet theme instead of removing the react defaultProps.

All this to say I don't think the comment is relevant for what we are trying to do here with removing the react defaultProps and we don't need to use the attrs method.

We did also test to confirm that the base theme is being passed through correctly to components that are not in a Grommet wrapper in react 18.3.0 without using attrs.

[undeletable]

@jcfilben

There is some confusing naming where we have defaultProps in the react sense and then we have defaultProps in Grommet that contains the base theme.

Actually these two entities are connected: Grommet's defaultProps is used as prototype for components' defaultProps.

Another hint for me was that component snapshots are way too different without both React's defaultProps and attrs usages, and almost equivalent with attrs.

[undeletable]

@jcfilben
As far as I see, there are two ways of using theme for components.

Way 1. Using inside the component function:

const Card = forwardRef(({ ...rest }, ref) => {
  const theme = useContext(ThemeContext) || defaultProps.theme;

  return (...);
});

Here, the case of using component outside of theme context is handled by using fallback vaue.No need to pass theme as default prop. I just externalized this logic in the custom hook.

Way 2. Using inside arguments of styled-components' template function:

const StyledMinute = styled.line.withConfig(styledComponentsConfig)`
  stroke-width: ${(props) => props.theme.clock.analog.minute.width};
  stroke: ${(props) =>
    normalizeColor(props.theme.clock.analog.minute.color, props.theme)};
  transition: stroke 1s ease-out;
`;

StyledMinute.defaultProps = {};
Object.setPrototypeOf(StyledMinute.defaultProps, defaultProps);

Here, the case of using component outside of theme context is handled by passing theme as default prop. So, if defaultProps is removed for such a component, we need another way to pass default theme as fallback. For such cases I'm using attrs(). In the most recent commit I just removed attrs() calls for the places where props.theme is not used.

Also, I've tried to remove all attrs() calls from my changes. As result, several test suites failed: this was caused by failed attempts to get properties of theme, which was not defined.

Please correct me if I'm wrong.

[jcfilben]

Thanks for the explanation, and sorry for the confusion on my end. Yesterday my team and I had tested in storybook by removing the Grommet wrapper and attrs and was still seeing the styles coming through so I was thinking we were good. But I just ran the unit tests and I see what you are saying about the difference in styles. I also had missed that we are currently using react defaultProps at the end of each Styled components file.

StyledMinute.defaultProps = {};
Object.setPrototypeOf(StyledMinute.defaultProps, defaultProps);

I think the direction you've taken with attrs makes sense.

[jcfilben]

If you update your branch with the latest changes from the master branch, the chromatic check should pass

[undeletable]

@jcfilben Thanks for the suggestion, but this branch is up-to-date with master, and chromatic is still failing. The error is 'Missing project token' - I believe that's because I haven't set up one for the fork.

[jcfilben]

Hmm you shouldn't need to set anything up on your end, the chromatic token should be accessible from grommet forks when they run on circleci. I'll look into what might be going wrong here

[jcfilben]

I'm going to try and push a no-op commit to your PR to see if I can re-trigger the chromatic step on circleci

edit: locally when I pull down this PR I have some tests failing, so I didn't end up pushing anything up yet

[undeletable]

@jcfilben

locally when I pull down this PR I have some tests failing

For me, some tests for Pagination are slowly running locally - both in master and my branch. I haven't checked yet what's causing the slowness.

[undeletable]

@jcfilben
From what I see, there's a different CircleCI project used for the fork:
https://app.circleci.com/pipelines/github/grommet/grommet
https://app.circleci.com/pipelines/github/undeletable/grommet
I assume here's the cause:

By default, CircleCI does not pass secrets to builds from forked PRs for open source projects and hides four types of configuration data:

Environment variables set through the application.

Deployment keys and user keys.

Passphraseless private SSH keys you have added to CircleCI to access arbitrary hosts during a build.

AWS permissions and configuration files.

Forked PR builds of open source projects that require secrets will not run successfully on CircleCI until you enable this setting.

[undeletable]

@jcfilben On my local env the following test is failing almost always:

  test('should update page range based on step prop dynamically changing', async () => {
    render(<TestComponent />);
    // Open select
    await userEvent.click(screen.getByRole('button', { name: /Open Drop/i }));
    // Click on the option '10'
    await userEvent.click(screen.getByRole('option', { name: '10' }));

    // Expect input value to be 10
    const updatedSelectButton = screen.getByRole('button', {
      name: 'Open Drop; Selected: 10',
    });
    expect(updatedSelectButton).toBeTruthy();
    expect(screen.getByText(`Showing 1-10 of 100`)).toBeTruthy();

    // Open select again
    await userEvent.click(screen.getByRole('button', { name: /Open Drop/i }));

    // Click on the option '50'
    await userEvent.click(screen.getByRole('option', { name: /50/i }));

    // Expect input value to be 50
    const updatedSelectButton1 = screen.getByRole('button', {
      name: 'Open Drop; Selected: 50',
    });
    expect(updatedSelectButton1).toBeTruthy();
    expect(screen.getByText(`Showing 1-50 of 100`)).toBeTruthy();

    // Open select again
    await userEvent.click(screen.getByRole('button', { name: /Open Drop/i }));

    // Click on the option '100'
    await userEvent.click(screen.getByRole('option', { name: /100/i }));

    // Expect input value to be 100
    const updatedSelectButton2 = screen.getByRole('button', {
      name: 'Open Drop; Selected: 100',
    });
    expect(updatedSelectButton2).toBeTruthy();
    expect(screen.getByText(`Showing 1-100 of 100`)).toBeTruthy();
  });

Thrown error:

"Exceeded timeout of 5000 ms for a test.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

Comparing to other tests in at least that module, there are many actions performed inside this one. Looks like this is the cause.

[jcfilben]

Hey @undeletable thanks for looking into this, we manually enabled the option so that we are passing the chromatic token to forked PR builds a few months ago so we should be good there. This has been working for other forked PRs but strangely not for this PR.

For the tests I am getting snapshot changes locally. I'm seeing if someone else on my team can run this locally to see if it is just something in my environment or if they are running into this as well.

[undeletable]

@jcfilben

For the tests I am getting snapshot changes locally

If I remember correctly, I was facing this several months ago after using npm for installing dependencies and running tests. When I use yarn, everything's fine (seeing yarn.lock in the repo, I assume this is expected).

we manually enabled the option so that we are passing the chromatic token to forked PR builds a few months ago

I'm not sure if it matters, but my fork was created prior to that.

[jcfilben]

Do you know why this snapshot is changing?

[undeletable]

I'm not sure about that yet. In fact, the changes are about reordering styles and removing the following one:

.c8:hover {
  box-shadow: 0px 0px 0px 2px #7D4CDB;
}

I'll check why that happens and if that's expected

[jcfilben]

Why do we need this change? Shouldn't Menu items = [] by default?

[undeletable]

At the moment I've added it, using Menu with no items prop passed resulted in test failure. I'll check if that's still the case and why, if yes.