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

Add support for ssl configuration to addListener #2243

Merged
merged 8 commits into from Feb 1, 2019
Merged

Conversation

cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Jan 31, 2019

Add public API support controlling SSL to the global notifier.

@realm-probot
Copy link

realm-probot bot commented Jan 31, 2019

Hey - looks like you forgot to add a T:* label - could you please add one?

@tgoyne
Copy link
Member

tgoyne commented Jan 31, 2019

This seems like a good time to switch to lumping all the configuration options for addListener into a single config object (i.e. something like addListener({serverUrl: '...', adminUser: user, filterRegex: '.*', sslConfiguration}, 'eventName', callback);). It's getting to a pretty unweildy number of paramters already, and it seems likely that we'll need to tack some more on in the future for more configuration.

If you don't want to deal with that now, the sslConfiguration parameter should come before the callback, as parameters after a callback are hard to read when using an inline function expression that may result in the final parameters being many many lines away from the function call. To make this a non-breaking change the implementation would check if the fifth argument is a function, and if so treat it as the callback rather than the sslConfiguration (this is kinda awkward but is a very common thing for JS libraries to do).

@cmelchior
Copy link
Contributor Author

Yeah. I had the same doubt as you about the API. Creating a new function with a single config object seems like a good approach and then we can deprecate the other method.

@cmelchior
Copy link
Contributor Author

@tgoyne This API does not appear to be used, and looking at the code I'm not sure it even works: https://github.com/realm/realm-js/blob/master/docs/sync.js#L141 Can it be safely removed?

@tgoyne
Copy link
Member

tgoyne commented Jan 31, 2019

Yes, that function is used and works. There's tests for it in realm-js-private.

lib/notifier.js Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
@cmelchior cmelchior merged commit 3f6214c into master Feb 1, 2019
@cmelchior cmelchior deleted the cm/listener-ssl branch February 1, 2019 12:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants