Skip to content

Commit

Permalink
followup from prior releases (#3813)
Browse files Browse the repository at this point in the history
* chore: add short contributor guide

* revert: "perf(FieldArray): add shouldComponentUpdate to cut down on unnecessary renders (#3784)"

Unfortunately this change introduced some unwanted behavior in
deeply-composed JSX that relied on state other than that provided
by Formik.

This reverts commit 22e236e.

* chore: fix playwright and update workflows

* chore: try a different fix

* chore: ignore webkit for now

* chore: add changeset for revert
  • Loading branch information
quantizor committed May 31, 2023
1 parent 2b194c2 commit 708bcb2
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 69 deletions.
5 changes: 5 additions & 0 deletions .changeset/rare-cars-compete.md
@@ -0,0 +1,5 @@
---
'formik': patch
---

Revert `FieldArray` "shouldComponentUpdate" performance optimization. As it turns out, it's a common use case to have JSX controlled via non-Formik state/props inside of `FieldArray`, so it's not safe to cancel re-renders here.
15 changes: 2 additions & 13 deletions .github/workflows/playwright.yml
Expand Up @@ -12,22 +12,11 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
cache: yarn
node-version: 18

- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUT

- uses: actions/cache@v3
id: yarn-cache # use this to check for `cache-hit` (`steps.yarn-cache.outputs.cache-hit != 'true'`)
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-
- name: Install dependencies
run: yarn
run: yarn install --frozen-lockfile

- name: Get installed Playwright version
id: playwright-version
Expand Down
18 changes: 3 additions & 15 deletions .github/workflows/release.yml
Expand Up @@ -13,22 +13,10 @@ jobs:
with:
fetch-depth: 0

- name: Use Node.js 18.x
uses: actions/setup-node@v3
- uses: actions/setup-node@v3
with:
node-version: 18.x

- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUT

- uses: actions/cache@v3
id: yarn-cache # use this to check for `cache-hit` (`steps.yarn-cache.outputs.cache-hit != 'true'`)
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-
cache: yarn
node-version: 18

- name: Install Dependencies
run: yarn install
Expand Down
15 changes: 2 additions & 13 deletions .github/workflows/test.yml
Expand Up @@ -20,25 +20,14 @@ jobs:
- name: Use Node.js ${{ matrix.node }}
uses: actions/setup-node@v3
with:
cache: yarn
node-version: ${{ matrix.node }}

- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUT

- uses: actions/cache@v3
id: yarn-cache # use this to check for `cache-hit` (`steps.yarn-cache.outputs.cache-hit != 'true'`)
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-
- name: Install deps, build, and test
run: |
node --version
npm --version
yarn --version
yarn --version
yarn install --frozen-lockfile
yarn test --coverage
env:
Expand Down
10 changes: 10 additions & 0 deletions packages/formik/README.md
Expand Up @@ -25,6 +25,16 @@
- Jared Palmer [@jaredpalmer](https://twitter.com/jaredpalmer)
- Ian White [@eonwhite](https://twitter.com/eonwhite)

## Contributing

This monorepo uses `yarn`, so to start you'll need the package manager installed.

To run E2E tests you'll also need Playwright set up, which can be done locally via `npx playwright install`. Afterward, run `yarn start:app` and in a separate tab run `yarn e2e:ui` to boot up the test runner.

When you're done with your changes, we use [changesets](https://github.com/changesets/changesets) to manage release notes. Run `yarn changeset` to autogenerate notes to be appended to your pull request.

Thank you!

## Contributors

Formik is made with <3 thanks to these wonderful people
Expand Down
22 changes: 0 additions & 22 deletions packages/formik/src/FieldArray.tsx
Expand Up @@ -27,8 +27,6 @@ export type FieldArrayConfig = {
name: string;
/** Should field array validate the form AFTER array updates/changes? */
validateOnChange?: boolean;
/** Override FieldArray's default shouldComponentUpdate */
shouldUpdate?: (nextProps: {}, props: {}) => boolean;
} & SharedRenderProps<FieldArrayRenderProps>;
export interface ArrayHelpers<T extends any[] = any[]> {
/** Imperatively add a value to the end of an array */
Expand Down Expand Up @@ -155,26 +153,6 @@ class FieldArrayInner<Values = {}> extends React.Component<
this.pop = this.pop.bind(this) as any;
}

shouldComponentUpdate(props: any) {
if (this.props.shouldUpdate) {
return this.props.shouldUpdate(props, this.props);
} else if (
props.name !== this.props.name ||
getIn(props.formik.values, this.props.name) !==
getIn(this.props.formik.values, this.props.name) ||
getIn(props.formik.errors, this.props.name) !==
getIn(this.props.formik.errors, this.props.name) ||
getIn(props.formik.touched, this.props.name) !==
getIn(this.props.formik.touched, this.props.name) ||
Object.keys(this.props).length !== Object.keys(props).length ||
props.formik.isSubmitting !== this.props.formik.isSubmitting
) {
return true;
} else {
return false;
}
}

componentDidUpdate(
prevProps: FieldArrayConfig & { formik: FormikContextType<Values> }
) {
Expand Down
15 changes: 9 additions & 6 deletions playwright.config.ts
@@ -1,4 +1,4 @@
import { defineConfig, devices } from '@playwright/test';
import { PlaywrightTestProject, defineConfig, devices } from '@playwright/test';

/**
* Read environment variables from file.
Expand Down Expand Up @@ -42,10 +42,13 @@ export default defineConfig({
use: { ...devices['Desktop Firefox'] },
},

{
name: 'webkit',
use: { ...devices['Desktop Safari'] },
},
// this one does not seem to be functional in github actions
!process.env.CI
? {
name: 'webkit',
use: { ...devices['Desktop Safari'] },
}
: null,

/* Test against mobile viewports. */
// {
Expand All @@ -66,7 +69,7 @@ export default defineConfig({
// name: 'Google Chrome',
// use: { ..devices['Desktop Chrome'], channel: 'chrome' },
// },
],
].filter(Boolean) as PlaywrightTestProject[],

/* Run your local dev server before starting the tests */
webServer: {
Expand Down

1 comment on commit 708bcb2

@vercel
Copy link

@vercel vercel bot commented on 708bcb2 May 31, 2023

Choose a reason for hiding this comment

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

Successfully deployed to the following URLs:

formik-docs – ./website

formik.org
formik-docs-git-main-formik.vercel.app
formik-docs.vercel.app
formik-docs-formik.vercel.app
www.formik.org

Please sign in to comment.