lp-belliot
commented
6 years ago Yeah, fair enough. So you're saying the use case is valid, but the implementation of checkHelper at the moment is too lenient?
Closed lp-belliot closed 6 years ago
lp-belliot
commented
6 years ago Yeah, fair enough. So you're saying the use case is valid, but the implementation of checkHelper at the moment is too lenient?
maritz
commented
6 years ago The changes in lib/relations.js are definitely correct.
The tests and changes for the helper are not quite right and if that's exactly your use-case for them, then your use-case is wrong in my opinion. But maybe I'm just missing information or misunderstanding something.
The error comes from my old concept for this though. Checking for equality instead of sameness was a bad choice and the only thing that really should be checked is that it's the same model definition (modelName in this case) and - if both have an id set - that the ids are the same as well. Checking for property equality means that default instances are always equal.
Changing this is a breaking change though, afaict. Not ideal. :-(
lp-belliot
commented
6 years ago Sorry for the delay. I think I confused myself, and written the test for a similar, but not identical use case. For me, the ID is always populated, so the ID check is where I expected to pass the helper's check.
Given your comments, removing the lines below and correcting the test to match my use case (IDs equal), we should be OK for the PR and avoid breaking changes?
maritz
commented
6 years ago Yes, I think so. While you're at it you can remove the (then) useless else if, if you want to. :)
This is - strictly speaking - a breaking change though. Thus publishing this on the v0.9/v1 line of releases is problematic imo. I'm not quite sure what to do yet.
One way could be to push my v2 to a v3 and then use v2 for it. But then we'd potentially run into the same issue in the future with other breaking change patches and once the ts rewrite version is published, we can't do that version pushing anymore.
Another way would be to give it a custom tag and not release it as stable, like version: v1.0.0-breaking-maintenance, tag: v1-maintenance. This way would mean anyone who has to stick to v1 but needs this patch can get it, but anyone who is still on v0.9 and doesn't need the patch can just ignore it.
lp-belliot
commented
6 years ago Done! The v1-maintenance tag should be fine I think. No point in pushing v2 to v3 for this change, you're just shifting the issue elsewhere.
lp-belliot
commented
6 years ago Awesome! When can we expect the merge?
lp-belliot
commented
6 years ago Sorry to chase this again. Any idea if we can merge this now?
maritz
commented
6 years ago No worries, I'm sorry that I'm not doing stuff here.
Currently I'm sick. Can't really focus on anything. Next week will be lots of catching up on work. Maybe on the weekend after. :-(
lp-belliot
commented
6 years ago All good! Just wanted to follow up as I hadn't heard anything since the PR was approved.
Get good before you get busy!
maritz
commented
6 years ago I was only looking at the checkmark in front of the travis-ci tests, but now while properly checking out the PR locally, I noticed that the CI stuff is currently broken and doesn't show errors.
The unlinkRelationChangeCleanup test still fails, because the roles don't have ids.
I've fixed both issues in #128 and will close this PR once that one is merged.
Hmm, having tested it a bit, I think there is a bigger/different problem.
The 2 instances may be equal at the time of the check, but they may differ later because they are not the same instance. They are unsaved (have no id) and could either be loaded from an id, filled with different values or linked to different other instances. Thus treating them as the same is wrong.
I think checkEqual how I implemented it way back then is just not very useful at all. It should probably just check if they are the same objects if they don't have ids set.