-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix many-to-many insert #7
base: master
Are you sure you want to change the base?
Conversation
@@ -23,7 +23,8 @@ export class ChangeSubscriber implements EntitySubscriberInterface<any> { | |||
if (ChangeSubscriber.disabled) | |||
return; | |||
|
|||
if (Reflect.hasMetadata("__track_changes", event.entity.constructor)) { | |||
if (event.entity && | |||
Reflect.hasMetadata("__track_changes", event.entity.constructor)) { |
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.
Is this a linting error or purposely on a new line?
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.
It's a new line on purpose - since the second line is kind of long, I think it helps readability.
However, note that the weird, long spacing on that second line is an artifact of how GH renders tabs (this project uses tabs for indentation, not spaces). So, if you open this in VSCode, it actually looks like this:
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.
Lets keep style based changes out of this PR for now, the next line is even longer, so this change would only make sense if we were to apply this style to the entire project.
The tab's don't really work well, it's usually tab for indentation and spaces for alignment.
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 don't see any issues here, but I think @DropsOfSerenity has the most context for this repo.
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.
Nice work catching that edge case! I just have a few changes to request before we can get this in. Also really appreciate the added tests!
@@ -23,7 +23,8 @@ export class ChangeSubscriber implements EntitySubscriberInterface<any> { | |||
if (ChangeSubscriber.disabled) | |||
return; | |||
|
|||
if (Reflect.hasMetadata("__track_changes", event.entity.constructor)) { | |||
if (event.entity && | |||
Reflect.hasMetadata("__track_changes", event.entity.constructor)) { |
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.
Lets keep style based changes out of this PR for now, the next line is even longer, so this change would only make sense if we were to apply this style to the entire project.
The tab's don't really work well, it's usually tab for indentation and spaces for alignment.
const related = await relatedFactory.createMany(3); | ||
|
||
// This inserts records into the join table implcitly created for MTM | ||
// relationships. These should not be tracked, and should not fail either. |
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.
Should not fail should be expressed in test rather than comment. Probably we should use chai-as-promised
here and do something like:
await expect(entity.save()).to.be.fulfilled;
Changes
ChangeSubscriber
to check whether an entity is being inserted.Purpose
Closes #6, allowing consumers to save many-to-many relations.