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

Breaking change to functionality in version 8 #152

Closed
redgeoff opened this issue Apr 28, 2019 · 15 comments
Closed

Breaking change to functionality in version 8 #152

redgeoff opened this issue Apr 28, 2019 · 15 comments

Comments

@redgeoff
Copy link

I want to start off by saying that sift rocks and it has been super helpful in mson!

Unfortunately, I recently tried to upgrade from v7 to v8 and there appears to be a massive breaking change, i.e. you now need to specify all the attributes in the query before anything is matched.

Take the example:

  console.log([{
    foo: 'bar',
    yar: 'nar'
  }].filter(
    sift({ foo: 'bar' })
  ));

with sift v7.0.1 and earlier, you get the output of [ { foo: 'bar', yar: 'nar' } ], but with v8.0.1 and later, you get []

To get an output of [ { foo: 'bar', yar: 'nar' } ] with v8.0.1, you need to include all the properties, e.g.

  console.log([{
    foo: 'bar',
    yar: 'nar'
  }].filter(
    sift({ foo: 'bar', yar: 'nar' })
  ));

Is this intentional or is this a bug?

@dweldon
Copy link

dweldon commented Apr 29, 2019

See the discussion for #151. I closed that issue because this change was intentional (I think it's based on #117). I'm not thrilled with it either.

For your example query to work, you'd want to do: .filter(sift({ foo: { $eq: 'bar' } }))

@redgeoff
Copy link
Author

Bummer! If this is intentional, I may need to fork v7 and start maintaining this fork.

@yoitsro
Copy link

yoitsro commented Apr 29, 2019

I'm also facing this issue. I think there was some misinterpretation in #117 which has caused this issue.

Whatever the case, for me, explicitly using $eq doesn't even fix it for me.

This functionality is now even more different from mongo.

@crcn
Copy link
Owner

crcn commented Apr 30, 2019

Sorry everyone, this issue should be fixed now in 8.3.2.

I think there was some misinterpretation in #117 which has caused this issue.

You're right, this was a misinterpretation 🙈. Object exactness should have only been applied to properties of the top-most query.

@yoitsro
Copy link

yoitsro commented Apr 30, 2019

I'll check out 8.3.2 now. Thank you for that and, generally, great work on this package 👏

@yoitsro
Copy link

yoitsro commented Apr 30, 2019

I can confirm, with a quick run with 8.3.2 just now, that this is fixed. I would recommend that others try it out too though before closing the issue /cc @redgeoff @dweldon.

Thank you for the speed of this fix @crcn!

@sakulstra
Copy link

8.3.2 resolved most issues we're facing, but there seem to be some leftovers

const array = [{ 'educations': [{ 'value': 'refa', 'unfinished': true }, { 'value': 'reno', 'unfinished': true }] }];
const query = {
  educations: {
    $elemMatch: {
      $or: [{
        value: 'refa',
        $or: [{ unfinished: true }]
      }, { 
        value: 'reno',
        $or: [{ unfinished: true }] }]
    }
  }
};
expect(array.filter(sift(query))).toHaveLength(1);

returns true on sift 7, but false on sift 8.3.2

@crcn
Copy link
Owner

crcn commented Apr 30, 2019

there seem to be some leftovers

Just pushed fixes to 8.3.3, let me know if it works.

@redgeoff
Copy link
Author

redgeoff commented Apr 30, 2019 via email

@sakulstra
Copy link

seems like the latest update fixed all tests in our not that huge test suite 👍!

@crcn
Copy link
Owner

crcn commented Apr 30, 2019

Thanks, Craig. Do you have these cases under test coverage?

You becha: db17350#diff-9079809255733a5a015a95f33de0b950R76

@crcn crcn closed this as completed Apr 30, 2019
@crcn crcn reopened this Apr 30, 2019
@crcn
Copy link
Owner

crcn commented Apr 30, 2019

@redgeoff is the issue fixed for you? If so I'll go ahead and close this ticket.

@redgeoff
Copy link
Author

redgeoff commented Apr 30, 2019 via email

@dweldon
Copy link

dweldon commented Apr 30, 2019

I can confirm that after reverting my changes and upgrading to 8.3.3, all of my tests pass. Thanks for the fix!

@redgeoff
Copy link
Author

redgeoff commented May 1, 2019

v8.3.3 appears to have fixed the problem. Thanks @crcn!

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

No branches or pull requests

5 participants