Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

Refactor connect functionality #280

Closed
wants to merge 3 commits into from
Closed

Conversation

anonrig
Copy link

@anonrig anonrig commented Jan 27, 2018

This pull request promises 2 things:

  • Add editorconfig (for people who don't know Pubnub employee uses 4 chars as default editor config)
  • Use Promise.all() instead of async.parallel() to handle connect event and return promise on connect to catch errors.

The main reason for opening this PR is that I couldn't find any way to catch the error inside connect.

  • Try catch doesn't work because connect event does async stuff and the upper scope doesn't know it.

@ianjennings
Copy link
Contributor

ianjennings commented Jan 29, 2018

Hey @anonrig, thanks for sending this in. The connection logic is much cleaner and helps us move toward removing async as a dependency.

I'm curious what error you were getting that was unable to be caught? You should be able to catch all errors via $.error events, and I'm wondering if that was working properly.

I'm currently working on refactoring connection logic and tests. I'd like to bring this in after so we can verify the tests. Since you have your own fork, I'm hoping this won't block you. Please let me know if otherwise.

@anonrig
Copy link
Author

anonrig commented Jan 29, 2018

@ianjennings It seems that my initial assumption was wrong, even though having a promise is a good thing, to catch the error in the same scope. On the other hand, the observer pattern made it harder to trace where the error originated from. My main purpose was to get notified by the result of these parallel tasks in the same scope.

Additionally I don't think that chat-engine is not react-native friendly at the moment.

The following screenshot was taken from our application. In development mode, react-native prints all console.error to the screen as a red screen of death. Somehow, PubNub servers return Internal Server Error and because of it chat-engine throws error and in that function there is a console.error

My questions are:

  1. How does Pubnub returns 500 internal server error? 🥇
  2. We should remove the console.error and do it console.warn for react-native friendliness (because warnings are printed to screen to the footer by default in react-native.)
  3. Will the refactorization include the usage of a different design pattern in chat-engine?

screen shot 2018-01-30 at 12 09 17 am

@ianjennings
Copy link
Contributor

ianjennings commented Jan 29, 2018

  1. How does Pubnub returns 500 internal server error?

I can't identify the source of your 500s. You may be able to get more information by checking the logs in PubNub functions.

  1. We should remove the console.error and do it console.warn for react-native friendliness (because warnings are printed to screen to the footer by default in react-native.)

Errors are thrown by default. You can disable thrown errors by supplying throwErrors: false into the ceConfig. See https://github.com/pubnub/chat-engine/blob/master/src/index.js#L12

  1. Will the refactorization include the usage of a different design pattern in chat-engine?

The design pattern will remain the same.

@ianjennings
Copy link
Contributor

Hey @anonrig, closing this as #289 has been merged to master. Please check that out and open an additional issue if you have further problems. I'll open an issue including your .editorconfig.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants