-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
@codydaig it doesn't affect any functionality though, so it's not a breaking change. |
@ilanbiala These passed, however, the cache didn't actually update it. I'll remove the cache temporarily from this PR then see if it still passes. |
I've been using Mongoose 4.2.3 since yesterday, and I've been testing quite a bit of the application. Everything seems to be working as expected, and I think I've actually seen some performance increases. I don't think it's an urgent need to upgrade, and I'd be fine with holding off on merging this until 0.5.0. But I wanted to share my thoughts on the matter. |
@codydaig @mleanos Express doesn't bump a minor version when they bump deps that have no effect on the functionality. Take a look at https://github.com/strongloop/express/blob/master/History.md#4132--2015-07-31 |
The reason I'd advocate to wait to merge this, is to give us time to test & make sure there aren't any critical bugs/limitations in this version of Mongoose. However, I don't think it would be too critical either way. If this version becomes an issue, then we can address it at that time. Perhaps, merging this in now would be good? To put this new version into the hands of our users that are using master or 0.4.2. Any thoughts on that point? |
@mleanos Mongoose is one of the top database packages on npm, I think bugs and limitations will have been found by the time we even get to publish 0.4.2. |
@ilanbiala I agree with that. One concern I have is the frequency at which Mongoose releases new versions. That's how this breaking change was introduced in the first place. With that said, I'm still fine with rolling this new version into the application now. We have pretty good test coverage right now, that should catch big bugs. For more advanced Mongoose features, we can rely on the community to catch issues there. |
directories: | ||
- node_modules/ | ||
- public/lib/ | ||
# cache: |
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.
Let's leave this to be modified in a separate PR.
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 know. I just removed it temporarily in this PR to make sure that it would still pass Travis.
1d291c9
to
09bc727
Compare
@ilanbiala I undid the changes from the cache on travis. I'm indifferent on when this gets merged in so I'll leave that up to you if you want it merged for 0.4.2 or for 0.5.0. |
@ilanbiala I'm going to recommend if this passes Travis that this gets held off until 0.5.0 since it also bumps a minor version.