-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature/#46 mongoose model definitions #61
Feature/#46 mongoose model definitions #61
Conversation
Master up to date
lib/mongoose.js
Outdated
/* Disabling because its a peer dependency */ | ||
/* eslint-disable global-require */ | ||
/* @ts-ignore */ | ||
const mongoose = require('mongoose'); |
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 you could require the mongoose
package at the top-level scope, just like you've done with m2s
. This will be more efficient since you won't have to re-require the mongoose
package every time the function is called (it will instead only be required once - when the file loads).
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.
This is a good one 😄 It was intentional. We could include the mongoose
package as a:
a) Peer dependency.
This was the original idea. The installation is optional for the client (could be using Sequelize for example).
As we can't handle errors at top-level scope, it would throw in case we require
an uninstalled package.
Even though it's less performant to re-require
inside the generateMongooseModelsSpec
scope, that function is intended to be called once and then use the same generated spec every time.
Ignore that it's also included as a Dev dependency, that was for basic_server.js
testing purposes.
b) Regular dependency.
The installation would be required (even if not needed by the client).
It could also lead to issues if the mongoose package version
from the client is different from the one used as a dependency on express-oas-generator
.
In this case, top-level scope require
would be correct.
For both cases, we should decide which is the minimum required version: https://mongoosejs.com/docs/compatibility.html
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 const m2s = require('mongoose-to-swagger');
be moved inside generateMongooseModelsSpec
as well to have it as peer dependency?
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
const m2s = require('mongoose-to-swagger');
be moved insidegenerateMongooseModelsSpec
as well to have it as peer dependency?
Sure, so should be go in both cases (mongoose
, mongoose-to-swagger
) with peer dependencies?
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.
They are all now peer dependencies (bson, mongoose, mongoose-to-swagger
). We should now agree the minimum required versions.
@@ -320,7 +325,7 @@ function handleRequests() { | |||
/** | |||
* @type { typeof import('./index').init } | |||
*/ | |||
function init(aApp, aPredefinedSpec = {}, aSpecOutputPath = undefined, aWriteInterval = 1000 * 10, aSwaggerUiServePath = 'api-docs') { | |||
function init(aApp, aPredefinedSpec = {}, aSpecOutputPath = undefined, aWriteInterval = 1000 * 10, aSwaggerUiServePath = 'api-docs', mongooseModels = []) { |
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.
Could you also include this functionality in the handleResponses
function? The init
one is only for backwards-compatibility and basic usage only; everything else uses handleResponses
and handleRequests
.
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.
Done! I've also updated both server_basic.js
and server_advanced.js
examples.
Thank you for your contribution! I haven't tried it our yet, but it looks promising:) |
lib/mongoose.js
Outdated
/* Disabling because its a peer dependency */ | ||
/* eslint-disable global-require */ | ||
/* @ts-ignore */ | ||
const mongoose = require('mongoose'); |
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 const m2s = require('mongoose-to-swagger');
be moved inside generateMongooseModelsSpec
as well to have it as peer dependency?
test/lib/mongoose_tests.js
Outdated
const mongooseModels = ['User', 'Student']; | ||
|
||
try { | ||
generateMongooseModelsSpec(mongooseModels); |
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.
using expect().to.throw(Error)
makes code more readable
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.
Done, now using expect().toThrowError(new RegExp())
lib/mongoose.js
Outdated
|
||
|
||
} catch (err) { | ||
throw new Error(`${err.message}`); |
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.
Looks like here any thrown exception is overwritten and rethrown, why is it needed? Would it be better to delete try/catch and allow original error to be thrown to simplify debugging?
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.
Yeah, I was wrapping with an extended error message before. Just removed it.
index.js
Outdated
if (validMongooseModels && !mongooseModelsSpecs) { | ||
mongooseModelsSpecs = generateMongooseModelsSpec(mongooseModels); | ||
} | ||
return mongooseModelsSpecs; |
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.
Is there any place where value returned from that functino is used? Maybe that return is not needed?
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.
Nope, it wasn't used. Returning void right now.
@maxiejbe thanks for your contribution! I'll test it locally and publish a new version soon. Will add a comment here after. |
version 1.0.25 is published, @maxiejbe could you please try it and confirm that it works as expected? |
Just tried an example project and working as expected! If Sending the argument to
First asking for the dependencies: |
A few comments only (but not related, it's up to
|
Should it be added to README.md? Something like |
Already done: |
Changes to include mongoose model as spec definitions (Issue #45):
mongooseModels?: Array<string>