fix(@rjsf/core): use name for additional properties #2878

ranihorev · 2022-06-09T06:44:03Z

Reasons for making this change

When rendering additional properties with title, the key of all properties appears to be the title of the property instead of the actual value of the key. Instead, we should first take the key in this case

Is there a reason to add a test for this?

Playground demo

TODO: fix tests if this looks like a reasonable solution

epicfaace · 2022-06-09T14:05:43Z

packages/core/src/components/fields/SchemaField.js

-    label = uiSchema["ui:title"] || props.schema.title || schema.title || name;
+    label = schema.hasOwnProperty(ADDITIONAL_PROPERTY_FLAG)
+      ? name
+      : uiSchema["ui:title"] || props.schema.title || schema.title || name;


I'm not sure quite what this is fixing. Can you add a test -- which should make it clearer the expected behavior?

You can see in the demo that the key input field contains the title of the field instead of the key of the addition property. I'll add some tests but I wanted to make sure that it makes sense first

Sorry about the late reply. Going to add some tests now, but to clarify the diff:

Before:

After:

Notice that the key is is propTitle which is the title of the field instead of the actual key fsdf

This is the demo playground

@epicfaace I finally added the test. Sorry about the delay again

heath-freenome · 2022-09-03T20:46:23Z

@ranihorev Please refactor this change onto the Typescript conversion of SchemaField and update the CHANGELOG.md file

heath-freenome · 2022-09-09T17:42:52Z

@ranihorev Did you have any questions or issues related to the refactor? If so I can be reached on discord

ranihorev · 2022-09-10T03:40:47Z

@heath-freenome I updated the PR. I've been struggling with running the playground locally (core updates are not reflected in the playground) which makes development much slower :(

heath-freenome · 2022-09-10T08:09:28Z

@heath-freenome I updated the PR. I've been struggling with running the playground locally (core updates are not reflected in the playground) which makes development much slower :(

Hmmm, if you run npm install and npm run build from the root directory the lerna setup should have made symbolic links in the playground. Are they not there? You can tell by looking in the packages/playground/node_modules/@rjsf directory. Once you start working, to see changes from one package reflected in others you'll need to run npm run build in the package you are working with

ranihorev · 2022-09-11T04:40:54Z

That's what I did :)
I see the dist files in the playground package if that's what you mean.
Do I need to run core in watch mode? I assumed that npm start is already watching and building the packages.

btw, I'm also seeing these errors when I run npm start (but the playground is running):

(typescript) Error: /Users/rhorev/react-jsonschema-form/packages/fluent-ui/src/FuiForm/FuiForm.tsx(2,38): semantic error TS7016: Could not find a declaration file for module '@rjsf/core'. '/Users/rhorev/react-jsonschema-form/packages/fluent-ui/node_modules/@rjsf/core/dist/index.js' implicitly has an 'any' type.

heath-freenome · 2022-09-12T06:27:12Z

That's what I did :) I see the dist files in the playground package if that's what you mean. Do I need to run core in watch mode? I assumed that npm start is already watching and building the packages.

btw, I'm also seeing these errors when I run npm start (but the playground is running):
(typescript) Error: /Users/rhorev/react-jsonschema-form/packages/fluent-ui/src/FuiForm/FuiForm.tsx(2,38): semantic error TS7016: Could not find a declaration file for module '@rjsf/core'. '/Users/rhorev/react-jsonschema-form/packages/fluent-ui/node_modules/@rjsf/core/dist/index.js' implicitly has an 'any' type.

Odd... Try running core in watch mode. Usually, I'm doing all the builds then starting playground.

ranihorev · 2022-09-12T08:56:12Z

How do you run all the builds? using npm run build?
I just did a fresh clone, then ran:

npm install
npm run build
npm start

And I'm getting a bunch of errors:

[...f/playground] ERROR in ../bootstrap-4/dist/index.js 7:2-62
[...f/playground] Module not found: Error: Can't resolve './bootstrap-4.cjs.development.js' in '/Users/xx/react-jsonschema-form/packages/bootstrap-4/dist'
[...f/playground]  @ ./src/index.js 8:0-61 87:11-26
[...f/playground]
[...f/playground] ERROR in ../chakra-ui/dist/index.js 7:2-60
[...f/playground] Module not found: Error: Can't resolve './chakra-ui.cjs.development.js' in '/Users/xx/react-jsonschema-form/packages/chakra-ui/dist'
[...f/playground]  @ ./src/index.js 9:0-57 91:11-24
[...f/playground]
[...f/playground] ERROR in ../fluent-ui/dist/index.js 7:2-60
[...f/playground] Module not found: Error: Can't resolve './fluent-ui.cjs.development.js' in '/Users/xx/react-jsonschema-form/packages/fluent-ui/dist'
[...f/playground]  @ ./src/index.js 5:0-57 95:11-24
[...f/playground]
[...f/playground] ERROR in ../material-ui/dist/index.js 7:2-62
[...f/playground] Module not found: Error: Can't resolve './material-ui.cjs.development.js' in '/Users/xx/react-jsonschema-form/packages/material-ui/dist'
[...f/playground]  @ ./src/index.js 3:0-56 99:11-21
[...f/playground]
[...f/playground] ERROR in ../mui/dist/index.js 7:2-54
[...f/playground] Module not found: Error: Can't resolve './mui.cjs.development.js' in '/Users/xx/react-jsonschema-form/packages/mui/dist'
[...f/playground]  @ ./src/index.js 4:0-48 103:11-21

It looks like lerna tried running the playground build before the other builds were finished. Not sure why that is. At least you figured out how to get by it.

heath-freenome · 2022-09-12T18:28:24Z

It looks like lerna tried running the playground build before the other builds were finished. Not sure why that is. At least you figured out how to get by it.

Does using the updated build command in this PR solve your issues?

packages/core/src/components/fields/SchemaField.tsx

ranihorev · 2022-09-12T21:04:53Z

@heath-freenome no, I still can't run the playground locally. I have no idea why...

heath-freenome · 2022-09-12T21:23:12Z

Reference

Hmmm, so it started breaking for me just now too?!

heath-freenome · 2022-09-13T00:56:01Z

Reference

Hmmm, so it started breaking for me just now too?!

Interesting... by switching back to main (yes, sorry master was renamed, but Github tells you how to fix that) and running npm run build playground compiled fine for me... Then I switched back to my branch and ran it again and it ran fine... Not sure why

packages/core/src/components/fields/SchemaField.tsx

ranihorev · 2022-09-13T04:01:23Z

I think that npm run start in the root folder is the culprit. It doesn't work.
However, if I run npm run start in playground then it runs just fine. What a waste of time... :(

I'm surprised it's working for everyone else though.

The next challenge, making the playground reload when updating core

heath-freenome · 2022-09-13T06:04:29Z

I think that npm run start in the root folder is the culprit. It doesn't work. However, if I run npm run start in playground then it runs just fine. What a waste of time... :(

I'm surprised it's working for everyone else though.

Yes, the npm start from the root directory probably worked just fine when there was core and playground back in that day... Today, with 11 other packages to monitor, it probably just doesn't work anymore or if it does it may crush the computer resources. I think we probably need to update the contributing documentation more helpful directions. I don't use it anymore. I just rebuild the packages I'm updating and redo npm start in the playground directory.

The next challenge, making the playground reload when updating core
Well, its a bit slow, but npm run build followed by npm start in playground will work. You could also temporarily edit the other themes package.json to remove the start command from them and then doing npm start from the root directory will only run core and playground

heath-freenome · 2022-09-14T06:31:52Z

@ranihorev Also, please update the CHANGELOG.md and add a new # 5.0.0-beta.8 with your change to ## @rjsf/utils

ranihorev · 2022-09-14T08:58:34Z

@heath-freenome I think that this PR is good to go :)

epicfaace reviewed Jun 9, 2022

View reviewed changes

ranihorev mentioned this pull request Jul 8, 2022

fix(utils): omit computedDefault of empty objects #2849

Closed

heath-freenome added the v5 refactor Needs refactor due to v5 breaking changes label Sep 3, 2022

heath-freenome added the p1 Important to fix soon label Sep 9, 2022

ranihorev force-pushed the fix-additional-property-key branch from 13cd7e6 to 1ecd13a Compare September 10, 2022 03:38

ranihorev force-pushed the fix-additional-property-key branch from 1ecd13a to 07f8f00 Compare September 12, 2022 09:23

heath-freenome reviewed Sep 12, 2022

View reviewed changes

packages/core/src/components/fields/SchemaField.tsx Outdated Show resolved Hide resolved

ranihorev force-pushed the fix-additional-property-key branch from 07f8f00 to f28d61f Compare September 12, 2022 21:04

heath-freenome reviewed Sep 13, 2022

View reviewed changes

packages/core/src/components/fields/SchemaField.tsx Outdated Show resolved Hide resolved

ranihorev force-pushed the fix-additional-property-key branch from 80d556f to d4cd15f Compare September 14, 2022 08:56

Rani added 3 commits September 14, 2022 11:57

fix(@rjsf/core): use name for additional properties

f0a4408

don't use hasOwnProperty

49dbc62

added changelog

473ce84

ranihorev force-pushed the fix-additional-property-key branch from d4cd15f to 473ce84 Compare September 14, 2022 08:57

heath-freenome approved these changes Sep 14, 2022

View reviewed changes

Merge branch 'main' into fix-additional-property-key

4f747ab

heath-freenome merged commit d6b3c1b into rjsf-team:main Sep 14, 2022

v1ack mentioned this pull request Sep 16, 2022

Title in additionalProperties replaces real value #2709

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(@rjsf/core): use name for additional properties #2878

fix(@rjsf/core): use name for additional properties #2878

ranihorev commented Jun 9, 2022 •

edited

Loading

epicfaace Jun 9, 2022

ranihorev Jun 11, 2022

ranihorev Jun 27, 2022

ranihorev Jul 3, 2022

heath-freenome commented Sep 3, 2022

heath-freenome commented Sep 9, 2022

ranihorev commented Sep 10, 2022

heath-freenome commented Sep 10, 2022 •

edited

Loading

ranihorev commented Sep 11, 2022 •

edited

Loading

heath-freenome commented Sep 12, 2022

ranihorev commented Sep 12, 2022 •

edited by heath-freenome

Loading

heath-freenome commented Sep 12, 2022

ranihorev commented Sep 12, 2022

heath-freenome commented Sep 12, 2022 •

edited

Loading

heath-freenome commented Sep 13, 2022

ranihorev commented Sep 13, 2022 •

edited

Loading

heath-freenome commented Sep 13, 2022 •

edited

Loading

heath-freenome commented Sep 14, 2022

ranihorev commented Sep 14, 2022

fix(@rjsf/core): use name for additional properties #2878

fix(@rjsf/core): use name for additional properties #2878

Conversation

ranihorev commented Jun 9, 2022 • edited Loading

Reasons for making this change

epicfaace Jun 9, 2022

Choose a reason for hiding this comment

ranihorev Jun 11, 2022

Choose a reason for hiding this comment

ranihorev Jun 27, 2022

Choose a reason for hiding this comment

ranihorev Jul 3, 2022

Choose a reason for hiding this comment

heath-freenome commented Sep 3, 2022

heath-freenome commented Sep 9, 2022

ranihorev commented Sep 10, 2022

heath-freenome commented Sep 10, 2022 • edited Loading

ranihorev commented Sep 11, 2022 • edited Loading

heath-freenome commented Sep 12, 2022

ranihorev commented Sep 12, 2022 • edited by heath-freenome Loading

heath-freenome commented Sep 12, 2022

ranihorev commented Sep 12, 2022

heath-freenome commented Sep 12, 2022 • edited Loading

heath-freenome commented Sep 13, 2022

ranihorev commented Sep 13, 2022 • edited Loading

heath-freenome commented Sep 13, 2022 • edited Loading

heath-freenome commented Sep 14, 2022

ranihorev commented Sep 14, 2022

ranihorev commented Jun 9, 2022 •

edited

Loading

heath-freenome commented Sep 10, 2022 •

edited

Loading

ranihorev commented Sep 11, 2022 •

edited

Loading

ranihorev commented Sep 12, 2022 •

edited by heath-freenome

Loading

heath-freenome commented Sep 12, 2022 •

edited

Loading

ranihorev commented Sep 13, 2022 •

edited

Loading

heath-freenome commented Sep 13, 2022 •

edited

Loading