Skip to content

Conversation

@rak3n
Copy link

@rak3n rak3n commented Oct 24, 2022

for issue #186

  • Add undefined check on onePropertyDiff return value, so as to return empty array from propertyDiff.

Copy link
Owner

@maritz maritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! ❤️

Could you add a test to make sure this stays working as intended after this PR? Something in typescript.test.ts that just makes sure void isn't the return type or value.

Do you think this would require a major version bump according to semver?

@rak3n
Copy link
Author

rak3n commented Oct 26, 2022

Hey @maritz,
many thanks for reviewing, have added test and dropped void from return type.
This will modify the way propertyDiff is used in older code to handle no differences in property for a key, so I think it will be a major bump.

Comment on lines +395 to +412
test.serial(
'return type',
async (t) => {
const testInstance = await nohm.factory<UserMockup>('UserMockup');
const diffBeforeUpdate = testInstance.propertyDiff('name');

// update the property
testInstance.property('name','testName');
const diffAfterUpdate = testInstance.propertyDiff('name');

t.deepEqual(diffBeforeUpdate, [], 'return without undefined');
t.deepEqual(diffAfterUpdate, [{
after: 'testName',
before: 'defaultName',
key: 'name',
}], 'return with difference in property');
},
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think this test would not have failed previously, right? The only real change is the return type of an item inside the array. So you'd have to make a test that loops over the diff and explicitly tries to access an item in it.

Something like

diffBeforeUpdate.forEach((diffItem) => { 
  // should never be called, but also shouldn't cause a TS error
  console.log('diff had item', diffItem);
  t.fail('should never reach this');
});

Copy link
Author

@rak3n rak3n Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, the resultant from the earlier version was returning [undefined] (as the JS func which doesn't return anything is undefined for return), and hence this test would have failed for deepEquals when compared with [] on the previous version. Happy to make change if that's not the case here

@maritz
Copy link
Owner

maritz commented Oct 26, 2022

This will modify the way propertyDiff is used in older code to handle no differences in property for a key, so I think it will be a major bump.

Yeah, I was thinking so as well. I'm kind of hesitant to just publish a v3 only for this rather minimal change. But on the other hand I can't really promise when other bigger changes are coming.

I guess I'll add this and some dependency updates into a v3 soon and not hold it up for other stuff.

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.

3 participants