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

feat: Add support for $eq in LiveQuery #7607

Closed

Conversation

sadakchap
Copy link
Member

@sadakchap sadakchap commented Oct 5, 2021

New Pull Request Checklist

Issue Description

As per this issue, currently it is not possible to combine equalTo clause with any other clauses and vice-versa using Parse-SDK-JS. After adding support for combining equalTo clause with other clauses in Parse-SDK-JS's pr.

We need to add support equalTo for LiveQuery, so that it triggers correct events for client.

Related issue: #7606

Approach

add support for $eq query in LiveQuery's matchKeyConstraints.

TODOs before merging

  • Add test cases

@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 5, 2021

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

@sadakchap sadakchap changed the title feat(LiveQuery): Support $eq feat: add support for $eq in LiveQuery Oct 5, 2021
@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #7607 (c4a2ef9) into alpha (a5ffb95) will decrease coverage by 7.12%.
The diff coverage is 83.33%.

❗ Current head c4a2ef9 differs from pull request most recent head dd3ce1a. Consider uploading reports for the commit dd3ce1a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            alpha    #7607      +/-   ##
==========================================
- Coverage   93.95%   86.82%   -7.13%     
==========================================
  Files         183      183              
  Lines       13655    13667      +12     
==========================================
- Hits        12830    11867     -963     
- Misses        825     1800     +975     
Impacted Files Coverage Δ
src/LiveQuery/QueryTools.js 94.05% <83.33%> (-0.68%) ⬇️
src/Adapters/Storage/Mongo/MongoCollection.js 4.76% <0.00%> (-92.86%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 11.99% <0.00%> (-81.38%) ⬇️
src/Adapters/Cache/RedisCacheAdapter.js 12.28% <0.00%> (-75.44%) ⬇️
src/Adapters/Files/GridFSBucketAdapter.js 10.65% <0.00%> (-68.86%) ⬇️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js 35.48% <0.00%> (-62.37%) ⬇️
src/Adapters/Storage/Mongo/MongoTransform.js 48.51% <0.00%> (-39.60%) ⬇️
src/Adapters/Files/GridStoreAdapter.js 13.04% <0.00%> (-33.34%) ⬇️
src/Routers/SessionsRouter.js 65.71% <0.00%> (-25.72%) ⬇️
src/Routers/AggregateRouter.js 88.00% <0.00%> (-12.00%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5ffb95...dd3ce1a. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Oct 13, 2021

@sadakchap I have followed the dashboard "is null" filter issue until here:

parse-community/parse-dashboard#1721 > parse-community/Parse-SDK-JS#1373 -> #7607

Is this PR the current stopper?

@sadakchap
Copy link
Member Author

Is this PR the current stopper?

I think so, with this PR, also all failing test should pass in parse-community/Parse-SDK-JS#1373. Then we can make changes accordingly in parse-community/parse-dashboard#1721 to get "is null" filter for dashboard.

@mtrezza
Copy link
Member

mtrezza commented Oct 13, 2021

Do you need a hand with this PR, or this ready for review?

@sadakchap
Copy link
Member Author

Do you need a hand with this PR, or this ready for review?

I think it is ready for review.

@mtrezza
Copy link
Member

mtrezza commented Oct 13, 2021

Oh even better, I'll take a look..

@mtrezza
Copy link
Member

mtrezza commented Oct 13, 2021

Could you merge master into this and add tests for LiveQuery?

@mtrezza
Copy link
Member

mtrezza commented Oct 30, 2021

@sadakchap Just a friendly ping, if you could add the tests that would be great, then we can merge and close the dashboard null filter issue as well. 🙂

@mtrezza
Copy link
Member

mtrezza commented Nov 17, 2021

@sadakchap please let me know when this is ready for review.

@mtrezza
Copy link
Member

mtrezza commented Jan 1, 2022

@sadakchap is this PR ready for review?

@sadakchap
Copy link
Member Author

@sadakchap is this PR ready for review?

Yes @mtrezza, I believe it is ready to be reviewed now.

@mtrezza mtrezza requested a review from a team January 6, 2022 13:39
@@ -255,6 +255,71 @@ describe('matchesQuery', function () {
expect(matchesQuery(img, q)).toBe(false);
});

it('matches on queries with new format #parse-SDK-JS/pull/1373', function () {
Copy link
Member

@mtrezza mtrezza Jan 6, 2022

Choose a reason for hiding this comment

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

How is this related to "add support for $eq in LiveQuery"? This is not a LiveQuery test. Is this a test for parse-community/Parse-SDK-JS#1373 which is a separate issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since in parse-community/Parse-SDK-JS#1373, we are changing the format of query. There are certain LiveQuery tests(parse-sdk-js) which are failing because parse-server doesn't support new query format for LiveQuery.

So, we added support for new query format in LiveQuery in parse-server.

Copy link
Member

@mtrezza mtrezza Jan 8, 2022

Choose a reason for hiding this comment

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

Not sure I follow; is parse-community/Parse-SDK-JS#1373 a requirement for this PR? I don't think so, otherwise the tests in this PR wouldn't already pass.

Copy link
Member Author

@sadakchap sadakchap Jan 9, 2022

Choose a reason for hiding this comment

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

Actually its the other, please follow this parse-community/Parse-SDK-JS#1373 (comment)

Copy link
Member

@mtrezza mtrezza Jan 12, 2022

Choose a reason for hiding this comment

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

So I understand that the tests in parse-community/Parse-SDK-JS#1373 will only pass when this PR (#7607) has been merged. But they relate to 2 different features:

  • add support for $eq in LiveQuery
  • add support to "combine equalTo clauses with [other] clauses"

So why is parse-community/Parse-SDK-JS#1373 mentioned in this test case if this PR (#7607) has no dependency on parse-community/Parse-SDK-JS#1373, only the other way around?

Copy link
Member Author

@sadakchap sadakchap Jan 19, 2022

Choose a reason for hiding this comment

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

@mtrezza sorry, I could not understand you properly, could you please elaborate a bit more?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it seems these are 2 different features:

Even if parse-community/Parse-SDK-JS#1373 never gets merged, we would still want to merge #7607 as a feature, right? So I don't understand why the tests in #7607 mention parse-community/Parse-SDK-JS#1373:

it('matches on queries with new format #parse-SDK-JS/pull/1373', function () {

@cbaker6
Copy link
Contributor

cbaker6 commented Jul 19, 2024

@mtrezza i think you can close this via #8614

@mtrezza
Copy link
Member

mtrezza commented Jul 21, 2024

Closing via #8614

@mtrezza mtrezza closed this Jul 21, 2024
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: add support for $eq in LiveQuery feat: Add support for $eq in LiveQuery Jul 21, 2024
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.

None yet

3 participants