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.

Sign up for GitHub

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

Jump to bottom

Fix: Realtime transport should fire events on a serial queue #578

Merged
merged 12 commits into from
Feb 5, 2017
Merged

Fix: Realtime transport should fire events on a serial queue #578

merged 12 commits into from
Feb 5, 2017

Conversation

ricardopereira
Copy link
Contributor

@ricardopereira ricardopereira commented Jan 16, 2017 •
edited
Loading

It wasn't firing transport events when the Realtime object was created on a background queue.
UPDATE: Rest was with the same problem.

@ricardopereira ricardopereira force-pushed the fix-577 branch 2 times, most recently from ea57a5f to 2851625 Compare January 18, 2017 15:09
tcard
tcard approved these changes Jan 18, 2017
View reviewed changes
Copy link
Contributor

@tcard tcard left a comment

Choose a reason for hiding this comment

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

Looks good, but can we add a test for this?

Source/ARTRealtime.m
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0), _eventQueue, ^{
// Wait until the current event is done.
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
ARTEventListener *listener = [_internalEventEmitter once:^(ARTConnectionStateChange *change) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we always create the listener, ie. why the condition on line 409 has been moved below 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.

@tcard Ah, that's wrong. Will fix it. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 380e6c7.

@ricardopereira ricardopereira mentioned this pull request Jan 18, 2017
Closed
@ricardopereira
Copy link
Contributor Author

Test added a1a77d1.

@ricardopereira
Copy link
Contributor Author

:/

Failing tests:
-[ARTRealtimePresenceTest testLeaveAndGet]
-[RealtimeClientChannel Channel__attach__should_transition_the_channel_state_to_FAILED_if_ATTACHED_ProtocolMessage_is_not_received()]
-[RealtimeClientConnection Connection__basic_operations_should_work_simultaneously()]
-[RealtimeClientConnection Connection__close__if_CONNECTING__do_the_operation_once_CONNECTED()]
-[RealtimeClientConnection Connection__connection_request_fails__connection_attempt_fails_for_any_recoverable_reason()]
-[RealtimeClientPresence Presence__leave__should_leave_the_current_client_from_the_channel_and_the_data_will_be_updated_with_the_value_provided()]
** TEST FAILED **

I'll check what's wrong.

mattheworiordan
mattheworiordan reviewed Jan 19, 2017
View reviewed changes
Copy link
Member

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

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

Looks good, although I can't really comment on the iOS specific details. It would be nice, as @tcard has said, to have one test that covers this background behaviour though.

Scripts/pre-commit.sh
@@ -0,0 +1,11 @@
#!/usr/bin/env bash
set -eu
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to commit 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.

Yes, it helps a lot when I only run one spec and I don't want to commit that to the server.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but that's for you and no else no? You leave here, but it's not that normal to add these utilities that you like to a shared repo

@ricardopereira
Copy link
Contributor Author

@mattheworiordan I already added a test: a1a77d1.

@ricardopereira
Copy link
Contributor Author

Can I squash?

@mattheworiordan
Copy link
Member

@ricardopereira but a1a77d1 is in the Examples folder? What has that to do with tests? The name of that file btw is pretty horrific Examples/Tests/TestsTests/TestsTests.swift! Has that got anything to do with Tests? 😏

mattheworiordan
mattheworiordan approved these changes Jan 23, 2017
View reviewed changes
Copy link
Member

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

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

LGTM, but I have to defer to @tcard for final say here

Scripts/pre-commit.sh
@@ -0,0 +1,11 @@
#!/usr/bin/env bash
set -eu
Copy link
Member

Choose a reason for hiding this comment

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

Ok, but that's for you and no else no? You leave here, but it's not that normal to add these utilities that you like to a shared repo

@ricardopereira ricardopereira merged commit f81703c into master Feb 5, 2017
@ricardopereira ricardopereira deleted the fix-577 branch February 5, 2017 15:32
maratal pushed a commit that referenced this pull request Jul 19, 2023
@tomczoink
fixes #578 - JSBin code examples for JWT
bc8b08f 
Also adds {{API_KEY_SECRET}} as a variable in docs. Also seperately added to our JSBin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattheworiordan mattheworiordan mattheworiordan approved these changes

@tcard tcard tcard approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ricardopereira @mattheworiordan @tcard