-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix: remove merge-deep
from the validation
#67
Conversation
Ran the tests locally with all of the same versions as below (and 23), all of the tests passed (except for node 16). Versions used to test:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR! merge-deep
is unnecessary nowadays and has some require magic conflicting with some js bundlers. I pulled the branch and tests against Node.js v18, v20, and v22 were successful.
The tests are failing due to a newer version of |
I updated all of the dependencies and updated the tests, I can make the changes part of this PR or make a new PR with the changes to the tests. Aside from the changes to |
@@ -37,7 +36,7 @@ module.exports = function makeValidatorMiddleware (middleware, schema, opts) { | |||
let validate | |||
|
|||
function makeValidator () { | |||
const reqSchema = merge({}, BASE_REQ_SCHEMA) | |||
const reqSchema = structuredClone(BASE_REQ_SCHEMA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I just wonder if a more simple approach to this would be enough? I am not opposed to this, but since we are not doing this for the specific reason structuredClone
is meant to serve I wonder if there are any unexpected consequences of this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go with this for now and we can decide after the work to support v5 if it is good to keep.
The tests are broken on |
We used to use
merge-deep
to assign parameters to a new object; however, we can usestructuredClone
to do the same thing and remove the dependency onmerge-deep
.structuredClone
only works on node 17+ but node 16 reached EOL in September 2023 per https://endoflife.date/nodejs and https://nodejs.org/en/blog/announcements/nodejs16-eol.