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

chore: add retrieveSchema at Form state to memoize the result of schemUtils.retrieveSchema #3970

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

igorbrasileiro
Copy link
Contributor

@igorbrasileiro igorbrasileiro commented Nov 21, 2023

Reasons for making this change

This PR focus on improve the onChange performance avoiding recalculate the retrievedSchema multiples times when Form has liveValidate prop true.

Before:
At the image below from a profiling of a keystroke on a field, the onChange function call retrieveSchema 2 and one more time during the update of the form component is, at getSnapshotBeforeUpdate. The time of onChange (~260ms) + getSnapshotBeforeUpdate (~166ms) represents 65% of task duration (~685ms)
image

After:
As you can see at the profiling below, the retrieveSchema function isn't called at onChange and getSnapshotBeforeUpdate. The result is, the onChange took ~100ms and the getSnapshotBeforeUpdate took ~87ms, this represents ~28% of the task duration ~652ms.
image

This PR follows the same idea from this one #3959

If your PR is non-trivial and you'd like to schedule a synchronous review, please add it to the weekly meeting agenda: https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@igorbrasileiro
Copy link
Contributor Author

igorbrasileiro commented Nov 21, 2023

I'm facing issues to run lerna, that's hanging. @heath-freenome or @zxbodya can you help me?

image

Versions
node: v18.12.1
npm: 9.4.0

OS: macOS 13.4.1 - m1 pro

@heath-freenome
Copy link
Member

I'm facing issues to run lerna, that's hanging. @heath-freenome or @zxbodya can you help me?

image

Versions node: v18.12.1 npm: 9.4.0

OS: macOS 13.4.1 - m1 pro

Do the other commands (i.e. npm run build, npm run test, etc) work?

@igorbrasileiro
Copy link
Contributor Author

Do the other commands (i.e. npm run build, npm run test, etc) work?

No, they don't. The script for each package works ok, but from lerna doesn't.

@igorbrasileiro
Copy link
Contributor Author

Related to: #1961 (comment)

@igorbrasileiro
Copy link
Contributor Author

@heath-freenome using npx i got a better info, the issue seems be related to Nx.
image

Then i removed the nx.json and entry from lerna.json and everything worked well.

if (omitExtraData === true && liveOmit === true) {
const retrievedSchema = schemaUtils.retrieveSchema(schema, formData);
const pathSchema = schemaUtils.toPathSchema(retrievedSchema, '', formData);
_retrievedSchema = schemaUtils.retrieveSchema(schema, formData);
Copy link
Contributor

@nickgros nickgros Nov 22, 2023

Choose a reason for hiding this comment

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

Would this work if you declared the newState object outside of its current conditional block?

Suggested change
_retrievedSchema = schemaUtils.retrieveSchema(schema, formData);
_retrievedSchema = newState?.retrievedSchema ?? schemaUtils.retrieveSchema(schema, formData);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I run the test, it fails on some tests with {"omitExtraData":true,"liveOmit":true}. Not fully sure, but probably, because the schema is old with unnecessary properties. 4 tests failed

@heath-freenome heath-freenome merged commit d7b59a3 into rjsf-team:main Nov 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants