Skip to content

Conversation

peterdotjs
Copy link
Contributor

throw new Error('LiveQueryController must implement open()');
}
if (typeof controller.close !== 'function') {
throw new Error('LiveQueryController must implement disconnect()');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: change to LiveQueryController must implement close()

@peterdotjs
Copy link
Contributor Author

Updated per comments.

@andrewimm
Copy link
Contributor

This all looks good to me. I'll do one last pass, then merge into the 1.8 release branch, followed by merging that into master. If the rye tests I just landed all pass, I'll cut a second RC on npm, which we can test as a standalone package. If that all looks good, we can bump the version number to 1.8.0 and push that out to everyone on npm.


// If we can not find Parse.liveQueryServerURL, we try to extract it from Parse.serverURL
if (!liveQueryServerURL) {
let host = url.parse(CoreManager.get('SERVER_URL')).host;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should really be a regex. There's no need to import the entire url browserify module, it's too much overhead (I know we've discussed this at least once before :).
It seems like what we really want is something like:

let host = CoreManager.get('SERVER_URL').replace(/^https?:\/\//, '');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I remember. I didn't write this part but I did miss it in the review so good catch. The url package wasn't even in package.json so this part was a miss overall.

andrewimm added a commit that referenced this pull request Mar 18, 2016
@andrewimm andrewimm merged commit a0d4a6e into release-1.8.0 Mar 18, 2016
@andrewimm
Copy link
Contributor

Made the changes, everything is merged. You can fetch the latest version of 1.8.0 with npm install parse@1.8.0-rc2

@peterdotjs peterdotjs deleted the peterjs.liveQuery branch March 18, 2016 18:50
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