-
Notifications
You must be signed in to change notification settings - Fork 96
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
Update to ES6 #33
Update to ES6 #33
Conversation
import errorHandler from './error-handler'; | ||
|
||
// Use native promises | ||
mongoose.Promise = global.Promise; |
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.
We could probably push this onto the user and document it. It will then allow them to use whatever promise lib they want.
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 the times for being that flexible about a Promise library are over. We're using ES6 so you'll get native promises. The Polyfill will always provide them and Node 5 comes with them already anyway (I think).
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.
Hmm. I agree, however if we are to enforce native promises and we want to keep the same API everywhere then we either need to document that the user should do this or get the user to pass in the mongoose object. Which one do you prefer?
Personally I think I like the first option better.
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 being able to define your own promise implementation is a better choice and allows maximum choice for the developer to use what they need or want.
Not sure what versions of node this project is expecting to support over time, but working from the Node LTS guide here:
https://github.com/nodejs/LTS/
If you'd like to support the maximum number of current LTS versions in the wild, you still need to support v.12 till April next year.
If my assumptions up until now are correct, you need the option to pass in your own promise library to support v.12.
I'm not claiming to be an expert though, so please double check my quick research.
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.
@jamesjnadeau You're right but the babel polyfill already has native promises covered. The tests are all passing all the way back to v0.10 so we are good there.
Thanks for weighing in! It's nice to have some fresh eyes on this stuff.
I'll have a look at the skipped tests and I'll quickly see how much work it would be to remove all the connection logic and have the user pass it in directly. Since this will be a new major release anyway I think it would be a good time to do that as well (I'm also changing that for MongoDB). I think the readme is still missing some documentation for how to extend the service as an ES6 class and the pagination options (all of which can probably just be copied from https://github.com/feathersjs/feathers-sequelize/#extending). I can add that as well. |
@daffl I'm already in the process of restructuring the mongoose connection stuff, so please don't touch it. I agree that now is the right time to change the module inputs. I'll deal with finishing off this PR today, including the missing documentation. The examples will need to change and I need to add some more documentation around validation, etc. |
Sounds good. Are we going to keep the Schema initialization or require users to do the same thing? |
We'll require users to do it I think. I just want to make sure that we can still do validations, statics/instance methods, our hooks and then mongoose hooks. I'm sort of doing this in conjunction with prototyping the database work in the generator to make sure this is going to work well. |
This PR does quite a bit:
options
param on initializationfeathers-service-tests
feathers-errors
Refs: feathersjs/feathers#182 and feathersjs/feathers#170