copleykj / socialize-messaging

A messaging package for Meteor
MIT License
72 stars 16 forks source link

Options at "conversations" publishing is ignored + Date client side #2

Closed markudevelop closed 9 years ago

markudevelop commented 9 years ago

Hi,

this.subscribe('conversations', {limit:1}); is ignored, It works fine on the messagesFor Publication.

Also there is a bigger issue I think with the message dates. When I chat and send new messages (using newMessage) there is a wrong order for a moment, because the client-side time and server-time is different so it's get sorted in the wrong order until simple-schema updates it via server new Date(). So typing quickly with different users timezones/times etc is a problem.

I'm not really sure if there is an easy solution for the dates can you force new message to be on top/bottom of the {{#each}} loop? Or set new date on client to some specific value?

Or just use https://github.com/mizzao/meteor-timesync?

copleykj commented 9 years ago

Ah, yes.. I seems when I switched to tmeasday:publish-with-relations I missed the options for the conversations query.. I'll get a fix out for that ASAP.

As for the dates, that could be a very annoying UI bug. Originally I hadn't added the date on the client side, but later decided to as a fix for another issue I was having. I think that mizzao:timesync or ground:servertime would probably be the best fix for this.. I'll have a look at both their API's in more detail and publish a fix for this with the fix for the limit on the conversations publication.

markudevelop commented 9 years ago

@copleykj Hey, I just checked timesync seems heavy, ground is more lightweight but it calls the server we need speed. I think a better option will be to keep the users time zone offset in the meteor users collection (and publish it) moment().zone() this sounds like a better way?

copleykj commented 9 years ago

@voidale I'm not sure I like that storing the timezone is the best solution, but I'm also thinking that just using a server time package won't be a proper fix either.. Maybe a combination of a server time package and storing the date in UTC would be a better solution.. I'm open to suggestions if anyone else has any.

As for the server time packages I'm leaning towards ground.. It looks as if we would only have to get the server time at startup and then calls to ServerTime.now() would be fast.

copleykj commented 9 years ago

I pushed a fix for the publication.. I should be able to get to the other issues tomorrow morning and I'll publish an updated package.

copleykj commented 9 years ago

@voidale I just published a new release which definitely fixes the publication issue. Give the date issue a thorough test verify that it fixes the issue at hand. I ended up creating a new lighter weight package to compensate for the server time difference.. I liked the ground:servertime package API but not the fact that it required the ground:store package which is unnecessary for this use case.

markudevelop commented 9 years ago

@copleykj Hey Thanks for such a quick fix :) I just tried the ServerTime package it seems to return wrong date.

ServerTime.date() Mon May 11 2015 09:06:58 GMT+0200 (Central Europe Daylight Time) and my server console returns on "date" Mon May 11 03:14:53 EDT 2015 It's GMT-0400

Calling Meteor.call("getServerTime" on the clients seems to return wrong date, Not sure why. But checked the mongodb and the message saved date is GMT -0400.

copleykj commented 9 years ago

Sorry about that.. I had an absolutely ridiculous error in the ServerTime.now() calculation.. I've updated the server-time package and updated the version dependency in messaging..

markudevelop commented 9 years ago

@copleykj I'm not sure if this problem unique to my setup. But the server side method gets the wrong date. It's working fine for you? Meteor.call("getServerTime", function(error,serverTimeStamp){console.log(error); console.log(serverTimeStamp)}) on the browser console returns wrong date.

copleykj commented 9 years ago

The getServerTime method just returns Date.now() so the serverTimeStamp should log as a millisecond timestamp.

markudevelop commented 9 years ago

@copleykj yeah I converted it at http://www.epochconverter.com/ and it's GMT +2 which is wrong. Is it correct one for you?

copleykj commented 9 years ago

Works perfectly on my end, and I'm not sure how it could be any other way except if there is another package that is overriding the method and returning an improper value.

markudevelop commented 9 years ago

@copleykj where do you test it locally? I just created a new empty meteor project + meteor add socialize:server-time and in the browser console ServerTime.date() it's wrong it's on a digitalocean ubuntu.

copleykj commented 9 years ago

So far I've tested locally and on meteor.com servers.

markudevelop commented 9 years ago

@copleykj http://servertime.meteor.com/ ServerTime.date() wrong for me how about for you? Or am I missing something this should be the server time right not mine? :)

copleykj commented 9 years ago

Yes, when I run it, I get 1431361151978 and the Epoch Converter gives me back the following...

GMT: Mon, 11 May 2015 16:50:23 GMT Your time zone: 5/11/2015, 12:50:23 PM GMT-4:00 DST which is proper for my timezone

copleykj commented 9 years ago

Running ServerTime.date(), I'm getting back Mon May 11 2015 12:54:42 GMT-0400 (EDT) which is proper

copleykj commented 9 years ago

Ok, seems that the issue doesn't exist for me because the time difference is so close.. I'll have to contemplate this and I'll see about fixing asap

copleykj commented 9 years ago

ok, after thinking about this a bit... I think what you may be perceiving as an issue is actually just the natural conversion of the unix epoch timestamp to your local time. Timestamps being GMT based are automatically converted to your local time when constructing a new Date(), so the same timestamp on a machine with it's timezone set to GMT +1:00 would construct a different local representation then a machine set to GMT -5:00 even though they essentially refer to the same exact time.

So this package should fix an improperly set server time if that is the issue that is causing your UI to be finicky. If this doesn't fix the UI problem then we may be looking at something different altogether.

markudevelop commented 9 years ago

@copleykj I'm really not sure about this one it's definitely odd that's it gets converted to something if we store it as a static string should be fine?, That's the whole problem for the UI. My new chat messages appear on top first for a moment and then shuffled to the bottom (new ones should be at bottom). The problem is at the client side because after the collection gets back the new message from server it has different date (correct one) and goes back to the bottom. basically latency compensation is against me this time. Either I have to disable client side collection or get fixed time from server ignore any conversation on client side with users time.

copleykj commented 9 years ago

As a quick check, if you go to the browser console and type ServerTime._timeDifference what does it return?

markudevelop commented 9 years ago

@copleykj ServerTime._timeDifference 14520

copleykj commented 9 years ago

ok, so yeah, a 14 second time difference could cause some issues, but I would think this should fix it.. Is your app on a server that I can access and try to determine the issue.. Unfortunately I don't have an easy way check this out since all the servers I have access to are within a couple hundred milliseconds of my local machines time.

markudevelop commented 9 years ago

@copleykj my server is also the same timezone as yours, I think changing the PC time should work? I will try to change GMT-4:00 see if this fixes my issue

copleykj commented 9 years ago

Changing the timezone on your machine unfortunately won't fix the issue due to the fact that all dates are GMT based which is universal the world around. On May 11, 2015 3:07 PM, "voidale" notifications@github.com wrote:

@copleykj https://github.com/copleykj my server is also the same timezone as yours, I think changing the PC time should work? I will try to change GMT-4:00 see if this fixes my issue

— Reply to this email directly or view it on GitHub https://github.com/copleykj/socialize-messaging/issues/2#issuecomment-101018974 .

markudevelop commented 9 years ago

@copleykj I definitely lost on how those dates work, So I will get you a server on Europe this should cause you to see the problem?

copleykj commented 9 years ago

No, it's not the timezone that is the issue, but the 14 second GMT based time difference. I'll see about starting up a DO vps shortly and deploying an app there. If need be I can manually set a time difference to test things out.

markudevelop commented 9 years ago

@copleykj I have created a droplet where you can debug this one: IP Address: 46.101.50.222 Username: root password: your nick name here :)

copleykj commented 9 years ago

So I didn't have time to do much more than deploy a skeleton app to and run stuff through the command line, adding a conversation and then adding messages to the conversation and watching them change to verify that they weren't coming back from the server with different values.. All seems to be working even with an almost 35 minute time difference manually set on the server.

markudevelop commented 9 years ago

@copleykj It's coming from the server fine. The problem is when the message is inserted to the client side. SimpleSchema/Collection2 autovalue inserts the message to client side first after a "moment" it gets updated from the server. If you query it manually you will probably miss it. Well it's getting too much complicated so sorry for getting your time wasted, I will just publish fixed server time and add message by server time that will solve the problem.

copleykj commented 9 years ago

Right, so the way I tested was to watch the last message on a conversation using a Tracker.autorun and logging out the message object.

Tracker.autorun(function(){
    console.log(Meteor.conversations.findOne().lastMessage());
});

This will log the last message out to the console anytime the last message changes, including when the latency compensation causes the date to come back with a different value from the server after simple-schema causes it to change.

Then I call Meteor.conversations.findOne().sendMessage("Message Text") to add a new message to the conversation. When I do this it logs the new message object once to the console. If there were a discrepancy in the times that would cause a UI flicker then it would log twice, once for the insert on the client and once when the server sent the document with the new date that had been inserted server side.

markudevelop commented 9 years ago

@copleykj I see, well when I get this one fixed I will let you know what was the cause maybe I'm doing something wrong (hopefully) Thanks a lot for your help :)

copleykj commented 9 years ago

@voidale ok, I've deployed an application with a bit of an interface to test with on the DO server you provided.. Its pretty basic and adding conversations and participants is still done through the console, but I've set up a conversation and sent a couple messages back and forth to test it out a bit and it seems to work as expected. Give it a workout and see if you can get it to give you the same issue you are having. You can log in with the email voidale@test.com password abcdefg or copleykj@gmail.com with the same password.

markudevelop commented 9 years ago

@copleykj just tried and it work as expected, Just needed to hit the messages limit (10) first only then you can see the issue. So just to be sure you can try writing 2 messages quickly now and if it works for you. It's my fault and I'm sorry not really sure if server-time package is actually not needed because of my mistake?

copleykj commented 9 years ago

So this is working without the UI issue for you?

copleykj commented 9 years ago

As for server-time package being needed, I think that it's a good addition being that the messaging package does add the date on the client side first as a way to mitigate the same UI issue when sorting but not having any date at all until the server sends back the new date.

markudevelop commented 9 years ago

@copleykj yeah it works fine :) no issues.

copleykj commented 9 years ago

Ok. I've created a new repo and pushed out the code I had deployed to your DO droplet. Maybe you can get an idea of how to fix your UI issues by looking at the code. UI code is located in the views folder and is separated into pages and partials.

markudevelop commented 9 years ago

@copleykj that will be really helpful, Thanks!

copleykj commented 9 years ago

You've very welcome