search

geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

autoIncrementId = true should make typeof model.id === 'number' #206

Open danfinlay opened 10 years ago

danfinlay commented 10 years ago

Right now even if autoIncrementId = true, a model's ID returns a string, which makes for some pretty strange comparisons, since foreign ID keys are now numbers, but personal IDs are now stringified.

mde commented 10 years ago

I haven't seen this happen -- I'm getting numbers for all the ids. Can you provide a repro case?

HitDaCa commented 10 years ago

Yes, I the have the same problem. When using the MySQL adapter, even when setting autoIncrementId = true directly in the model core, it always returns an ID string.

mde commented 10 years ago

These changes are all only in the master branch, not in a release. Are you guys talking about a released version?

HitDaCa commented 10 years ago

I'm using npm install model@latest. What version would you advise using?

-----Original Message----- From: Matthew Eernisse Sent: 16/06/14 07:59 a.m. To: geddy/model Cc: Dave Cartwright Subject: Re: [model] autoIncrementId = true should make typeof model.id ==='number' (#206)

These changes are all only in the master branch, not in a release. Are you guys talking about a released version?

— Reply to this email directly or view it on GitHub.

mde commented 10 years ago

Ugh, sorry, this is all work that's been going on in the master branch -- it's a pretty big change that will require a new release. Master branch is passing all tests, so it's probably fine to use. But it is under development, so there's always the possibility of breakage. I was hoping to land @ben-ng's nested-include feature in the next release -- but either way, this will be getting pushed out soon into a full release.

HitDaCa commented 10 years ago

No worries, if I find some time I might write a test for checking the id type, it seems an important feature for SQL adapter as indexing chracters on large one to many tables is just a pain. Any idea what previous release this works in?

-----Original Message----- From: Matthew Eernisse Sent: 16/06/14 08:35 a.m. To: geddy/model Cc: Dave Cartwright Subject: Re: [model] autoIncrementId = true should make typeof model.id ==='number' (#206)

Ugh, sorry, this is all work that's been going on in the master branch -- it's a pretty big change that will require a new release. Master branch is passing all tests, so it's probably fine to use. But it is under development, so there's always the possibility of breakage. I was hoping to land @ben-ng's nested-include feature in the next release -- but either way, this will be getting pushed out soon into a full release.

— Reply to this email directly or view it on GitHub.

ben-ng commented 10 years ago

autoincrement and nested-include are pretty big changes, please notify us a few days before announcing the new release so we can QA that stuff!

mde commented 10 years ago

@ben-ng, absolutely. Will go through the usual process of cutting a release branch and letting people bang on it for a while before pushing it out. Do we have just that one regression that's a release-blocker? Any progress now that there's a failing test?

@HitDaCa I don't think it every worked the way it should. But since we're close to cutting a release branch, you can just use master until we push this stuff out.

ben-ng commented 10 years ago

@mde not yet. i thought of a problem in the solution in master, will continue discussion over in #212