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

#3059: Upgrade to RJSF5 #4913

Merged
merged 31 commits into from Dec 4, 2023
Merged

#3059: Upgrade to RJSF5 #4913

merged 31 commits into from Dec 4, 2023

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented Dec 30, 2022

What does this PR do?

Reviewer Tips

Remaining Work

Future Work

  • Attempt to replace ajv 6 with CloudFlare’s validator in our repo
  • Potentially upstream it (could be a substantial amount of work)

Checklist

  • Do some smoke tests
  • Double-check updated snapshots
  • Designate a primary reviewer

package.json Outdated
"@rjsf/bootstrap-4": "^5.0.0-beta.15",
"@rjsf/core": "^5.0.0-beta.15",
"@rjsf/utils": "^5.0.0-beta.15",
"@rjsf/validator-ajv6": "^5.0.0-beta.15",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm only updating rjsf here, not the validator. Let's ensure it works first and then move to CloudFlare’s validator, not ajv 8

import React from "react";
import FormPreviewFieldTemplate, {
type FormPreviewFieldProps,
} from "./FormPreviewFieldTemplate";
import styles from "./FormPreviewBooleanField.module.scss";

const RjsfBooleanField = RjsfTheme.fields.BooleanField;
const RjsfBooleanField = getDefaultRegistry().fields.BooleanField;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Double check that this produces the output we expect

According to the upgrade guide, they reduced duplicated code so it's possible that these fields are shared now. Maybe the theme needs to be applied separately though

@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (838a5ad) 70.96% compared to head (23f398a) 70.96%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/bricks/renderers/CustomFormComponent.tsx 37.50% 5 Missing ⚠️
src/components/formBuilder/FieldTemplate.tsx 68.75% 5 Missing ⚠️
...omponents/formBuilder/DescriptionFieldTemplate.tsx 72.72% 3 Missing ⚠️
src/pageEditor/slices/editorSlice.ts 66.66% 2 Missing ⚠️
...ionConsole/pages/integrations/IntegrationsPage.tsx 0.00% 1 Missing ⚠️
src/pageEditor/slices/editorSliceHelpers.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4913   +/-   ##
=======================================
  Coverage   70.96%   70.96%           
=======================================
  Files        1204     1206    +2     
  Lines       37237    37260   +23     
  Branches     6972     6983   +11     
=======================================
+ Hits        26424    26442   +18     
- Misses      10813    10818    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@twschiller twschiller added the do not merge Merging of this PR is blocked by another one label Dec 30, 2022
@fregante
Copy link
Collaborator Author

fregante commented Jan 4, 2023

So this seems to work already. Could you review the code changes, the snapshots and double-check that the document builder works as expected? I did some testing and saw zero issues.

This might be a good time to re-confirm that we're going to stay on RJSF and not migrating to greener pastures (if any exist.) For example you could look at the buglist to see if you see any dealbreakers https://github.com/rjsf-team/react-jsonschema-form/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3Abug

@twschiller
Copy link
Contributor

This might be a good time to re-confirm that we're going to stay on RJSF

We're staying on RJSF. I don't know of any greener pastures

Looks like they'll be releasing final 5.0 version this month (hopefully): rjsf-team/react-jsonschema-form#3144

@twschiller
Copy link
Contributor

twschiller commented Jan 23, 2023

Potential bugs/regressions:

image

image

  • Form Description no longer supports markdown (I'm 85% sure it did before, but I haven't checked on main yet)

image

Confirmed working

  • File upload widget (we had fixed a bug in the bootstrap 4 widget) in a previous PR
  • Marking a field as required
  • Email field validation
  • Number field shows correct widget
  • Default value
  • Paragraph field type
  • Field description supports markdown
  • Dropdown field
  • Dropdown field with labels
  • Date/time field
  • Date field

@twschiller
Copy link
Contributor

@fregante see here: #4913 (comment)

I bumped to the latest beta. The story shot changes look fine to me. They just changed the id format for fields

Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

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

See regressions

@twschiller twschiller added this to the 1.7.20 milestone Jan 26, 2023
@fregante fregante removed their assignment Jan 28, 2023
@twschiller twschiller removed this from the 1.7.20 milestone Feb 10, 2023
@twschiller twschiller removed the do not merge Merging of this PR is blocked by another one label Feb 11, 2023
@twschiller twschiller marked this pull request as draft February 11, 2023 20:57
@fregante fregante assigned BLoe and unassigned twschiller Nov 24, 2023
@fregante fregante requested a review from BLoe November 24, 2023 07:45
@grahamlangford
Copy link
Collaborator

Looks good other than the failing test. I'll add it to my list to debug.

Copy link

github-actions bot commented Dec 4, 2023

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@fregante
Copy link
Collaborator Author

fregante commented Dec 4, 2023

:shipit:

@twschiller twschiller merged commit 5624ac3 into main Dec 4, 2023
17 checks passed
@twschiller twschiller deleted the F/deps/upgrade-to-rjsf-5 branch December 4, 2023 23:28
@grahamlangford grahamlangford added this to the 1.8.5 milestone Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants