Fixes LiveQuery behavior on multiple subscriptions #758
Conversation
|
Thank you for the PR. Test cases are needed. Check out the Contributors Guide |
|
As I peeked into the existing tests for LiveQuery, I'm a bit concerned that my PR is still a hack in it's current state. ParseLiveQuery being a mere proxy class for LiveQueryClient, i feel like it would be more relevant to implement the Promise-based workflow directly in the LiveQueryClient class, resolving promises after the server acknoledges any instructions. You are right to assume it might also break things for certain users, but I wonder how any developer could have been able to use the LQ functionality with the framework so far, considering how restricted the working usecase is. Maybe feedback from developers with actual experience with this functionality is required beforehand ? IMO, Promises are mandatory. |
Codecov Report
@@ Coverage Diff @@
## master #758 +/- ##
==========================================
+ Coverage 89.97% 90.25% +0.28%
==========================================
Files 54 54
Lines 4676 4640 -36
Branches 1076 1077 +1
==========================================
- Hits 4207 4188 -19
+ Misses 469 452 -17
Continue to review full report at Codecov.
|
|
@Amphaal I added a few tests to this. Can you check them and add anything you think is missing. |
| @@ -393,10 +393,9 @@ module.exports = { | |||
|
|
|||
| setLiveQueryController(controller: any) { | |||
| requireMethods('LiveQueryController', [ | |||
There was a problem hiding this comment.
can you help me understand what this change is please?
There was a problem hiding this comment.
Those function (subscribe, unsubscribe, open, close) already exists in the LiveQueryClient.
In the LiveQueryController those were just wrapper functions. Instead of using wrapper functions call the ones from the LiveQueryClient (Which you can get from the LiveQueryController).
setDefaultLiveQueryClient, getDefaultLiveQueryClient, _clearCachedDefaultClient already existed but weren't documented.
| liveQueryServerURL = protocol + host; | ||
| const serverURL = url.parse(CoreManager.get('SERVER_URL')); | ||
| const protocol = serverURL.protocol === 'http:' ? 'ws://' : 'wss://'; | ||
| liveQueryServerURL = protocol + serverURL.host + serverURL.pathname; |
There was a problem hiding this comment.
uh @dplewis...wait a sec...is this on the client? will the url class be available?
I'm so used to all the parse stuff being on the server, I get a little confused on what's where when it comes to the JS SDK...😬
There was a problem hiding this comment.
I forgot about that too. I keep forgetting about other environments besides node.
To answer your question I don't think so. I'll revert it in a PR.
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/API/URL
not sure what compatibility we're looking for.
|
@Amphaal Thanks for opening the PR. Sorry if I kinda took it over. We can look more deeply into the LiveQueryClient to see if there are improvements to be made. |
|
@dplewis You did great, I honestly don't have much time right now to dig back into Parse... This fix is a great addition to future release ! |
* ⚡️ Release v2.3.0 Closes: #687, #670, #723, #669 * Pin packages * (Jest) Fix deprecated setupTestFrameworkScriptFile * (Codecov) Fix coverage folders * Update Parse.Cloud.define docs * Add discource badge to Readme Pending #753 * add user subclass fix * add typescript to readme * typo * add discourse to js sdk * remove url module #758 (review) * Update CHANGELOG.md
Fixes #278