Skip to content
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

Change findById to findByPk #73

Closed

Conversation

valentijnnieman
Copy link

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
Copy link

@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
Copy link

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
Copy link

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
Copy link

tcrossrfid commented Oct 28, 2019

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
Copy link
Author

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
Copy link

mikbur commented Oct 30, 2019

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
Copy link

Any update on this?

@Sam-Dietrich
Copy link

Any idea when this will get merged in?

@valentijnnieman
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request - Model.findByPk
9 participants