-
Notifications
You must be signed in to change notification settings - Fork 40
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: validate API requests and responses according to the OpenApi spec #1579
Conversation
10ed66d
to
701d192
Compare
packages/backend/vite.config.js
Outdated
'@jsdevtools/ono': '@jsdevtools/ono/cjs/index.js', | ||
'ono': '@jsdevtools/ono/cjs/index.js', |
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.
curious why we need that ? these one are usually just for the local source code new links
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.
Ono (a dependency of express-openapi-validator) provides both versions, cjs and esm, and vite includes the esm one by default.
But the esm version includes this code, which fails because module.exports.default
is not defined:
// CommonJS default export hack
if (typeof module === "object" && typeof module.exports === "object") {
module.exports = Object.assign(module.exports.default, module.exports);
}
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.
shouldn't the extension be set as a module ?
type: "module" ?
I did that for the podman extension
https://github.com/containers/podman-desktop/blob/main/extensions/podman/package.json#L7
and we say we generate a cjs file
https://github.com/containers/podman-desktop/blob/main/extensions/podman/package.json#L13
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.
the rationale is that we have more and more libraries that are only esm
so instead of trying to grab the commonjs files, we should switch to be esm and allow to use esm easily as well
some libraries are now esm only
podman-desktop/podman-desktop@8e6debf
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.
@benoitf I just tested with type: "module"
(and removing the aliases), but the error is the same. The hack in the esm version of ono seems not compatible with vite
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 that we can't use cjs as for long term it'll be a burden
we may patch the module using JS-DevTools/ono#20 or getting rid of the lines ?
or switch to another library
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.
OK, I'll make changes to patch ono's esm version, so we can use it. Thanks @benoitf
Signed-off-by: Philippe Martin <[email protected]>
701d192
to
bbae16a
Compare
What does this PR do?
Validate API requests and responses according to the OpenApi spec
Using the
express-openapi-validator
needs to patch theono
dependency with JS-DevTools/ono#20We use the
@rollup/replace
plugin for this (another solution would be to useyarn patch
to patch thenode_modules
directly, but the project is currently using yarn v1.22, which does not include this command, available only starting from v3)What issues does this PR fix or reference?
Fixes #1578
How to test this PR?