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

Added sails socket connection mutex #27

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

t3rminus
Copy link

We were using Ember.RSVP.hash({ ..., ..., ... }) to load 3 models. When the page was loaded on this route, it attempted to connect to sails 3 times, because the CORS cookie request had not yet completed from the first request, before the second was initiated.

This caused some weird behaviour, and quite a few errors in the console...

This should resolve that by only allowing one method to actually request a connection, and queueing up the remaining callbacks for when the socket is finally connected.

t3rminus and others added 7 commits August 12, 2015 15:01
…s, and errors related to CORS cookie loading
	- Removed all references to typeKey
	- changed serializer to use new normalize syntax
	- Removed typeKey references
	- Changed serializer to use new normalize syntax
	- Updated the store mixing push method to use the new data structure
@mukk85
Copy link

mukk85 commented Aug 19, 2015

Also included ember-data 2.0 compatibility

@jelhan
Copy link

jelhan commented Sep 20, 2015

Using your PR together with @heartsentwined fixes for #27 ember-data-sails seems to be totally broken. Not sure if it's your PR or heartsentwined fixes.

Getting warnings like:

WARNING: Encountered "toggleProperty" in payload, but no model was found for model name "toggle-property" (resolved model name using client@serializer:sails:.modelNameFromPayloadKey("toggleProperty"))

Working fine using #29 together with heartsentwined fixes with ember data 1.13.8.

@mukk85
Copy link

mukk85 commented Sep 21, 2015

Are you using the REST adapter? There were no changes made to that as we are using sockets.

@jelhan
Copy link

jelhan commented Sep 21, 2015

I'm using SailsSocketAdapter. I have merged your PR with heartsentwined/ember-data-sails in latest master (see master branch in jelhan/ember-data-sails). Generated a brand new project using ember-cli 1.13.8 and SailsJS 0.11.0 I'm facing this issue.

@huafu
Copy link
Owner

huafu commented Oct 2, 2015

Hi @t3rminus, I am very sorry to answer only now. I just saw this PR after fixing some issues both related and unrelated to Ember version. Can you rebase this PR and send me an update here when done with my name tagged @ so that I can integrate this in a 0.0.18.

@t3rminus
Copy link
Author

@huafu Sorry for being so late myself. My coworker and I finally found some time to get this sorted out, and it should work for the 0.0.18 release now, along with ember-data 2.0

push: function (typeName/*, data, _partial*/) {
var res = this._super.apply(this, arguments), id, type, adapter;
if (this._pushSubscribes && res && (id = res.get('id'))) {
type = this.modelFor(typeName);
Copy link
Owner

Choose a reason for hiding this comment

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

You're missing the test on _pushSubscribes on the refactored code. I am on my phone and that is a long time I did not work on this addon but I believe it has to be there because depending on settings it should or not subscribe

@huafu
Copy link
Owner

huafu commented Nov 25, 2015

Thank you! I'll handle it asap, if you did not do the last tiny fix I'll do it and merge.
But thanks A LOT ;-)

Cody Mcculloch and others added 3 commits November 25, 2015 15:15
…int. In our case sails handles this automatically on the server
Use the `ember-data` initializer as a target instead of the soon to b…
@huafu
Copy link
Owner

huafu commented Sep 5, 2018

@t3rminus who is Cody? Anyway, @t3rminus and/or Cody are you interested in taking over this project?

@t3rminus
Copy link
Author

t3rminus commented Sep 5, 2018

@huafu Cody is @mukk85 . Not sure why his commits are not tracking his account. Will discuss with him whether taking over it is possible/feasible.

@huafu
Copy link
Owner

huafu commented Sep 5, 2018

@t3rminus was more like adding you as collaborators so that you could merge PRs

@huafu
Copy link
Owner

huafu commented Sep 5, 2018

anyway #43

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.

5 participants