Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Remove message content from JS client logging #1694

Merged
merged 2 commits into from
Mar 23, 2018

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Mar 23, 2018

@JamesNK JamesNK changed the base branch from dev to release/2.1 March 23, 2018 00:19
Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Looks good to me. @davidfowl if you want to introduce a flag to re-enable this, file an issue for that and we can track it later. This change is required to ship though, so I'd rather get it in ASAP.

@davidfowl
Copy link
Member

Looks good to me. @davidfowl if you want to introduce a flag to re-enable this, file an issue for that and we can track it later. This change is required to ship though, so I'd rather get it in ASAP.

Ummm, naw. See the problem is when you use binary you can't really use any other tools to look at the data, so I think the flag has to be added along with the feature.

@JamesNK
Copy link
Member Author

JamesNK commented Mar 23, 2018

Where should the flag exist? How does a dev set it?

@davidfowl
Copy link
Member

Where should the flag exist? How does a dev set it?

All of our ts flags are currrently on an options object passed into the hubConnection constructor. I imagine it would go there.

@analogrelay
Copy link
Contributor

See the problem is when you use binary you can't really use any other tools

What do you expect our logging to output? Base64? Hex?

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM I was concerned about logging after messages were deserialized by the hubconnection.

@JamesNK JamesNK merged commit d367bdc into release/2.1 Mar 23, 2018
@JamesNK JamesNK deleted the jamesnk/jslogging-removecontent branch April 2, 2018 00:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants