-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(server): check that serverMode implementation is correct #2051
base: master
Are you sure you want to change the base?
feat(server): check that serverMode implementation is correct #2051
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2051 +/- ##
==========================================
+ Coverage 92.77% 92.82% +0.04%
==========================================
Files 29 29
Lines 1149 1156 +7
Branches 327 329 +2
==========================================
+ Hits 1066 1073 +7
Misses 79 79
Partials 4 4
Continue to review full report at Codecov.
|
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 think it is extra checking and should be solved using schema-util
instead adding checks in source code, we should search way how we can integrate this in schema util
Switched to use of |
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.
Good job, one note
}, | ||
"required": ["constructor", "send", "close", "onConnection"], | ||
"additionalProperties": false | ||
} |
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 think we can have one schema (put this inside original schema) for better maintenance
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.
@evilebottnawi The problem is that I couldn't find a way to test the prototype of a function with schema-utils
when it matched "instanceof": "Function"
. I think it only looks at "properties"
if the thing in question matches "type": "object"
, hence why I pass ServerImplementation.prototype
into the 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.
Unless you just mean make options.json
into something like:
{
"defaultSchema": { ... },
"socketServerImplementationPrototypeSchema": { ... }
}
But that could be a breaking change if something else is using this 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.
hm we can rewrite schema-utils and allow to define own validator (where we can implement any logic), i will take care about this, let's keep this PR open, it is not high priority because we can improve documentation
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
This feature gives useful error messages when the user provides an incorrect
serverMode
implementation.Breaking Changes
None
Additional Info