search

BlinkUX / sequelize-mock

A simple mock interface specifically for testing code relying on Sequelize models
https://sequelize-mock.readthedocs.io
MIT License
139 stars 73 forks source link

Change findById to findByPk #73

Closed valentijnnieman closed 4 years ago

valentijnnieman commented 5 years ago

Hi,

This changes the findById method to findByPk, which is a change made in Sequelize v5. I realize there are other changes made from v4 to v5 that, if this repo chooses to support v5, should be implemented. However, this was an easy fix and something I personally needed, so I thought I'd create a PR anyway. This should close #71.

I cannot find much information on what versions of Sequelize this lib supports - and since this is currently version 0.10.2 I'm guessing no real plans have been made yet. If any plans are made, I'm more than happy to either add more changes to this PR or help in creating more PR's that make the switch from v4 to v5 as explained here: http://docs.sequelizejs.com/manual/upgrade-to-v5.html

Thanks for all your great work!

JustinMGaiam commented 5 years ago

@valentijnnieman thanks for getting that PR in and I would also be interested in v5 support as well. We use this for testing v4 heavily so that seems to be the currently supported version.

arctouch-ricardobonfim commented 5 years ago

For those of you who are still waiting for this merge, you can use a proxy from findByPk to findById... something like this: model.findByPk = (query) => model.findById(query)

stoked10 commented 4 years ago

Any update on when this will be pulled in? @arctouch-ricardobonfim do you have an example on how to use that proxy? I haven't been able to get it to work.

tcrossrfid commented 4 years ago

Any news on merging this in? Would be great...

BTW, the line in the comment above for proxying findByPk should be: model.findByPk = (id, opts) => model.findById(id, opts); Working for me, at least.

valentijnnieman commented 4 years ago

Sorry for the silence on this! It's been a hell of a summer. It's been a while since I looked at this - can this be merged as-is or would it need to include anything else?

mikbur commented 4 years ago

As many probably still use sequelize v4 this would be a breaking change for them. One solution would be to mark findByID deprecated as in late sequelize 4 versions.

big-kahuna-burger commented 4 years ago

Any update on this?

Sam-Dietrich commented 4 years ago

Any idea when this will get merged in?

valentijnnieman commented 4 years ago

I'm closing this as it's been a long while since I did this work and I don't have time to pick this up again in the near future.