fix: remove inbound message size validation #382
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Approved by Richie |
owenpearson
force-pushed
the
max_message_size_fix
branch
from
fd38d30 to
0e2b204
Compare
owenpearson
changed the title
owenpearson
changed the base branch from
main
to
remove-deprecated-recover-behaviour-test
owenpearson
force-pushed
the
max_message_size_fix
branch
from
0e2b204 to
e2764e6
Compare
ghost
approved these changes
Oct 3, 2022
QuintinWillison
suggested changes
Oct 3, 2022
Contributor
QuintinWillison
left a comment
QuintinWillison
left a comment
There was a problem hiding this comment.
I see we're mostly removing code here.
What I can't see is why we're making this change. There is no opening comment on this pull request that explains to reviewers or those looking back in future why the change is being made. Even the single commit only contains a single line:
fix: remove inbound max_message_size check
which pretty much matches the pull request title:
fix: remove inbound message size validation
What I would like to see, one or more of:
- Opening comment explaining why the change is being made (i.e. what is wrong in the current implementation)
- A
buglabel assigned to ensure it makes it into the changelog as such - A link to a GitHub issue that is
buglabelled, describing the problem with the current implementation - A link from this PR, or the linked GitHub issue, to other sources of information ... e.g. an " (internal)"-suffixed link to a Slack thread, perhaps
Member
Author
|
Good point @QuintinWillison, I've updated the description now |
QuintinWillison
approved these changes
Oct 4, 2022
Base automatically changed from
remove-deprecated-recover-behaviour-test
to
main
October 4, 2022 09:40
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #377
TO3l8specifies the requirement of the SDK to validate message size for outbound messages so the check for inbound messages is not needed. Moreover the existing implementation readsconnection_detailsfrom theMESSAGEprotocol message (which is always unset) and falls back to the library default, so doesn't work when an account has a configured max_message_size