Skip to content

(っ˘▽˘)っ ☁️☁️☁️ ⊂(◕。◕⊂) #2

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

Merged
merged 13 commits into from
Feb 22, 2017

Conversation

mmimeault
Copy link
Contributor

@mmimeault mmimeault commented Oct 7, 2016

(っ˘▽˘)っ ☁️☁️☁️ ⊂(◕。◕⊂)

@nlutsenko
Copy link

I love how this is using our commit style.
+1 just for that.

@flovilmart
Copy link

@nlutsenko we've learnt from the best!

@@ -19,8 +19,7 @@
jsonObject.put("op", "subscribe");
jsonObject.put("requestId", requestId);

ParseEncoder encoder = PointerEncoder.get();
JSONObject queryJsonObject = state.toJSON(encoder);
JSONObject queryJsonObject = state.toJSON(NoObjectsEncoder.get());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why NoObjectsEncoder? I was testing this, and wasn't able to get it to compare a pointer field to a ParseObject that represents the expected field value. Nothing got sent over the socket though, which prompted me to dig deeper. Turns out that uncovered a different issue, which I'm also fixing along the way. But when I changed this back to PointerEncoder, my .whereEquals("key", theParseObj) predicate suddenly started working correctly.

What was the reason for changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted that change. I thought I tested with the NoObjectsEncoder but seems not. Good catch.

}

private void sendSubscription(Subscription<T> subscription) {
sendOperationAsync(new SubscribeClientOperation<>(subscription.getRequestId(), subscription.getQueryState()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This swallows exceptions thrown by the ClientOperation. In my case, it was an IllegalArgumentException thrown by NoObjectsEncoder when given a ParseObject.

I have a local commit to fix this by dispatching it into HandleErrorCallback.onError().

Should I push those commits here, or wait for this initial code to get merged and then make a new PR for that?

Choose a reason for hiding this comment

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

Push à PR on that one would be ok.

Joe Hansche added 2 commits November 14, 2016 13:36
If the query can't be encoded (or any other checked or unchecked Exception
occurs), the async SubscribeClientOperation will swallow the exception,
leaving no way (even a log message) to see why it failed.

This creates a new LiveQueryException subclass (UnknownException) to
represent an unexpected RuntimeException that can then be handled by the
HandleErrorCallback#onError() callback.
@jhansche
Copy link
Contributor

jhansche commented Jan 5, 2017

What's going on with this project?

@jhansche
Copy link
Contributor

jhansche commented Feb 3, 2017

@flovilmart @nlutsenko @mmimeault
What exactly is the holdup on this repo? Anything I can do to help move it along?

@flovilmart
Copy link

@jhansche nothing's holding on our side, besides a thourough review of the core, and perhaps credentials to publish on a package manager . @mmimeault?

Joe Hansche added 3 commits February 3, 2017 11:38
This allows the results to be chained together via tasks, and also allows the
CurrentUserController to be overridden to provide additional functionality
for retrieving the session token.

This matches how the ParseCloud and ParseObject retrieve the session token.
@mmimeault
Copy link
Contributor Author

Hello, sorry for the downtime on this.
I'll make sure this still works first, and validate the NoObjectsEncoder vs Pointer.

@jhansche Could you please give more information about your problems you having other than the Encoder with this PR?

It would be nice to merge it in master and make it publicly available if it works.

Giving you an update really soon about the encoding problem.

@mmimeault
Copy link
Contributor Author

Everything seems to works really fine on my side. Seen your PRs.

Do you think we are ready to merge that in master? Or there is still things we should finish first?

@jhansche
Copy link
Contributor

Or there is still things we should finish first?

I've been using it with just the two changes in PR here for a couple months now. We haven't had any other issues aside from those two changes.

@mmimeault
Copy link
Contributor Author

awesome @jhansche, we will just need to configure the release and build check after that.

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.

4 participants