Skip to content

Conversation

@dvv
Copy link
Contributor

@dvv dvv commented Jun 8, 2011

No description provided.

@dvv
Copy link
Contributor Author

dvv commented Jun 8, 2011

The last three is of concern. Don't know so far how to remove commits...

Copy link
Contributor

Choose a reason for hiding this comment

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

i'll rather see "' + message + '" here instead of a JSON.stringify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if message is verbatim 'I like to collaborate to "3rd-Eden"'? ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

message.replace(/\"/g, '\\"') ;D? I rather prevent as much JSON.stringify's as possible as they are blocking actions.. There is no streaming JSON parser / stringifier yet ;9..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we stringify a short message once for a faulty connection. It's not used in normal message traffic. But I admit that, if we are sure we won't use dquotes in our error messages, your initial proposal saves a couple of ticks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going with 3rd-Eden's first approach here.

@dvv
Copy link
Contributor Author

dvv commented Jun 10, 2011

Please, consider applying the last three commits -- they are critical. TIA

Copy link
Contributor

Choose a reason for hiding this comment

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

The way this works changed recently. Please see up-to-date version.

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