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

Update README.md and set watermark based on provided options #66

Closed
wants to merge 3 commits into from

Conversation

EricDahlvang
Copy link
Member

Remove: Watermark with websocket will be supported in the future.

The future is now! (this has been addressed)

@EricDahlvang
Copy link
Member Author

Fix was rolled back. Closing PR for now. :(

@EricDahlvang
Copy link
Member Author

Fix is has now been deployed. Watermark with websockets is functioning as expected.

@EricDahlvang EricDahlvang reopened this Apr 13, 2018
@EricDahlvang
Copy link
Member Author

fix for microsoft/BotFramework-WebChat#850

@EricDahlvang EricDahlvang changed the title Update README.md Update README.md and set watermark based on provided options May 9, 2018
@nicolastomeo
Copy link

@EricDahlvang Nice work but I think the private attribute watermark should be '-' by default. Because now, if you use websockets (websocket = true) and don't initialize the value of the watermark you get the messages from the last watermark. But if you don't use websockets and don't initialize the value of the watermark you get the messages from the begining of the conversation.

@EricDahlvang
Copy link
Member Author

@nicolastomeo Thank you for pointing this out. I'll consult with some others, and get back to you.

@EricDahlvang
Copy link
Member Author

@nicolastomeo Apparently this is a modification the managers of this library are not interested in taking until there is a protocol change.

Closing this PR so as to not waste people's time further.

@nicolastomeo
Copy link

@EricDahlvang Thanks for the reply.

@bhushanvadgave
Copy link

Fix is has now been deployed. Watermark with websockets is functioning as expected.

@EricDahlvang Is this change merged to master or tagged?

@EricDahlvang
Copy link
Member Author

@bhushanvadgave No, this has not been merged. I am not a maintainer of this library, and the maintainers are not interested in taking this PR at this time. If you want to use WebSockets with watermark in DirectLineJS, you will need to make these changes in your own fork.

@bhushanvadgave
Copy link

@EricDahlvang Fine, thanks.

@EricDahlvang
Copy link
Member Author

@bhushanvadgave @nicolastomeo

fyi: Someone else had more luck getting this change into the library: #96

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.

3 participants