Skip to content

Remove enumNames #3031

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

Merged
merged 4 commits into from
Aug 23, 2022
Merged

Remove enumNames #3031

merged 4 commits into from
Aug 23, 2022

Conversation

nickgros
Copy link
Contributor

Reasons for making this change

See #3016

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

Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

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

Fix the snapshots so that the tests pass and you are good to go I believe

#### Alternative JSON-Schema compliant approach

JSON Schema has an alternative approach to enumerations using `anyOf`; react-jsonschema-form supports it as well.
JSON Schema supports the following approach to enumerations using `oneOf`/`anyOf`; react-jsonschema-form supports it as well.
Copy link
Member

Choose a reason for hiding this comment

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

Also consider adding the oneOf solution mentioned in #532 as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +57 to +65
if (
schema.enum &&
schema.enum.length === 2 &&
schema.enum.every((v) => typeof v === "boolean")
) {
enumOptions = [
{ value: schema.enum[0], label: schema.enum[0] ? "Yes" : "No" },
{ value: schema.enum[1], label: schema.enum[1] ? "Yes" : "No" },
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heath-freenome instead of changing the snapshots, I opted to maintain the existing behavior of labeling boolean enums as "Yes"/"No".

Copy link
Member

Choose a reason for hiding this comment

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

Keep this as it is, don't use enumNames

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we still need to check for user-supplied enumNames, but we can continue to not use enumNames for the [true, false] case

@nickgros nickgros merged commit eb16fe1 into rjsf-team:rjsf-v5 Aug 23, 2022
Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

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

@nickgros Per your message to Ashwin, I'm adding comments about what I feel aught to be restored and not restored

@@ -28,6 +28,10 @@ There are three new packages added in RJSF version 5:

### `@rjsf/core` BREAKING CHANGES

#### Support dropped for non-standard `enumNames` property
Copy link
Member

@heath-freenome heath-freenome Aug 24, 2022

Choose a reason for hiding this comment

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

@nickgros Change this to an indication of deprecation... maybe it doesn't belong under the breaking change, but still listed as something to be migrated.

Comment on lines +253 to +254
Gets the list of options from the schema. If the schema has an enum list, then those enum values are returned. The labels for the options will be the same as the `value`.
If the schema has a `oneOf` or `anyOf`, then the value is the list of `const` values from the schema and the label is either the `schema.title`, if it exists, or the value.
Copy link
Member

Choose a reason for hiding this comment

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

@nickgros Revert this, and add a note about the deprecation of enumNames, to be removed in next release

Comment on lines -66 to -82
This library supports a custom [`enumNames`](https://github.com/rjsf-team/react-jsonschema-form/issues/57) property for `enum` fields, which, however is not JSON-Schema compliant (see below for a compliant approach).
The `enumNames` property allows defining custom labels for each option of an `enum`:

```jsx
import validator from "@rjsf/validator-ajv6";

const schema = {
type: "number",
enum: [1, 2, 3],
enumNames: ["one", "two", "three"]
};

render((
<Form schema={schema} validator={validator} />
), document.getElementById("app"));
```

Copy link
Member

Choose a reason for hiding this comment

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

Either restore this, or maybe keep it deleted and instead add a note about enumNames being deprecated only

@@ -35,8 +35,33 @@ Here's a list of supported alternative widgets for different JSON Schema data ty
* `select`: a select box with `true` and `false` as options;
* by default, a checkbox is used

> Note: To set the labels for a boolean field, instead of using `true` and `false` you can set `enumNames` in your schema. Note that `enumNames` belongs in your `schema`, not the `uiSchema`, and the order is always `[true, false]`.
Copy link
Member

Choose a reason for hiding this comment

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

Keep this deleted.

Comment on lines +57 to +65
if (
schema.enum &&
schema.enum.length === 2 &&
schema.enum.every((v) => typeof v === "boolean")
) {
enumOptions = [
{ value: schema.enum[0], label: schema.enum[0] ? "Yes" : "No" },
{ value: schema.enum[1], label: schema.enum[1] ? "Yes" : "No" },
];
Copy link
Member

Choose a reason for hiding this comment

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

Keep this as it is, don't use enumNames

Comment on lines -70 to -71
enum: ["screen", "multiply", "overlay"],
enumNames: ["Screen", "Multiply", "Overlay"],
Copy link
Member

Choose a reason for hiding this comment

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

Do not restore this

@@ -91,7 +91,20 @@ export default {
title: "Custom select widget with options",
type: "string",
enum: ["foo", "bar"],
enumNames: ["Foo", "Bar"],
Copy link
Member

Choose a reason for hiding this comment

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

Do not restore this

Comment on lines -16 to -17
return schema.enum.map((value, i) => {
const label = (schema.enumNames && schema.enumNames[i]) || String(value);
Copy link
Member

Choose a reason for hiding this comment

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

Consider restoring the old hack that I removed since I'd like to not restore the change to the type. Also add a console.warn() about enumNames being deprecated, to be removed in the next major release

Comment on lines -12 to +14
* update this one type. Also, because we are supporting the non-standard `enumNames` prop in the code, define it here.
* update this one type.
*/
export type RJSFSchema = JSONSchema7 & {
enumNames?: string[];
};
export type RJSFSchema = JSONSchema7;
Copy link
Member

@heath-freenome heath-freenome Aug 24, 2022

Choose a reason for hiding this comment

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

Keep this change as is... let's not make the type encourage people to use enumNames

Comment on lines -10 to -22
const enumNameSchema = {
...enumSchema,
enumNames: ["Option1", "Option2", "Option3"],
};
expect(optionsList(enumSchema)).toEqual(
enumSchema.enum!.map((opt) => ({ label: opt, value: opt }))
);
expect(optionsList(enumNameSchema)).toEqual(
enumNameSchema.enum!.map((opt, index) => {
const label = enumNameSchema.enumNames[index] || opt;
return { label: label, value: opt };
})
);
Copy link
Member

Choose a reason for hiding this comment

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

Sigh, I suppose we'll restore this to, checking for console.warn()

heath-freenome pushed a commit to heath-freenome/react-jsonschema-form that referenced this pull request Aug 27, 2022
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.

2 participants