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

convert property in array type #314

Merged
merged 4 commits into from
Dec 6, 2022
Merged

convert property in array type #314

merged 4 commits into from
Dec 6, 2022

Conversation

DenisFerrero
Copy link

No description provided.

@@ -5,6 +5,16 @@
module.exports = function ({ schema, messages }, path, context) {
const src = [];

let sanitized = false;
if (schema.convert === true) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we shouldn't have to wrap the null and undefined values because they are no "real" values.
The current implementation will wrap the { roles: null } to { roles: [null] }

it("should wrap single value into array", () => {
const check = v.compile({ $$root: true, type: "array", items: "string", convert: true });
expect(check("a")).toEqual(true);
});
Copy link
Owner

Choose a reason for hiding this comment

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

Please add more test, to check the santitized value as well, and add test when the input:

  • an array with value,
  • an empty array
  • null
  • undefined

@DenisFerrero
Copy link
Author

Added null and undefined checks + deep tests as requested. The tests have been moved to the "Sanitizations" describe section as it was more meaningful based on the context

@icebob
Copy link
Owner

icebob commented Dec 6, 2022

Please note, I changed the nullish check to a simpler one because in this lib we aim to be the fastest, and creating an array for the check is very expensive:

image

By the way, thank you for your contribution!

@icebob icebob merged commit e2b0086 into icebob:master Dec 6, 2022
@icebob
Copy link
Owner

icebob commented Dec 17, 2022

Released

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