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

Inconsistency between afterLiveQueryEvent "type" and SDK #7022

Closed
dblythy opened this issue Nov 24, 2020 · 3 comments · Fixed by #7023
Closed

Inconsistency between afterLiveQueryEvent "type" and SDK #7022

dblythy opened this issue Nov 24, 2020 · 3 comments · Fixed by #7023
Labels
type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@dblythy
Copy link
Member

dblythy commented Nov 24, 2020

Is your feature request related to a problem? Please describe.

This is my fault, but basically, request.event in the afterLiveQueryEvent contains a first letter uppercase, where the JS SDK does not. E.g:

 Parse.Cloud.afterLiveQueryEvent('TestObject', req => {
      expect(req.event).toBe('Create');
});

vs

subscription.on('create', object => {});

This could be confusing and potentially lead to bugs. Infact, I even made the mistake for the docs:

Parse.Cloud.afterLiveQueryEvent('MyObject', async (request) => {
  if (request.event != "update") {
    request.sendEvent = false;
    return;
  }
  const object = request.object;
  const pointer = object.get("child");
  await pointer.fetch();
});

This example will never fire as .event is defined as:

let type;
if (isOriginalMatched && isCurrentMatched) {
   type = 'Update'; // 'Update', not 'update'
} else if (isOriginalMatched && !isCurrentMatched) {
   type = 'Leave';
} else if (!isOriginalMatched && isCurrentMatched) {
   if (originalParseObject) {
      type = 'Enter';
   } else {
      type = 'Create';
   }
} else {
   return null;
}

Describe the solution you'd like

Casing consistent across SDK and afterLiveQueryEvent, or at least documented that they are different.

@mtrezza mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed S4 labels Nov 24, 2020
@mtrezza
Copy link
Member

mtrezza commented Nov 24, 2020

Thanks for reporting.

I classified this as a bug with severity level 4 because:

  • this is an inconsistency with expectations that can be derived from related syntax in the Parse JS SDK
  • this is an inconsistency with the docs
  • the functionality itself is not impaired, albeit one would have to look into the source code to find the correct event spellings

The suggested solution would be to make the spelling consistent by changing it to lower case.
The bug was apparently introduced in #6859, parse-community/docs#773.

Would you be willing to open a PR?
We should also look at test cases that verify the correct spelling.

@dblythy
Copy link
Member Author

dblythy commented Nov 24, 2020

Agree with all the points you've raised. Only a minor nit that will cause inconvenience and confusion. Potentially breaking change for the next release though for those already utilising the trigger.

@mtrezza
Copy link
Member

mtrezza commented Nov 24, 2020

Yes, given that this feature has only been introduced about 1 month ago, I would assume that the breaking change impact is not that big of a deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants