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.

Sign up for GitHub

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

Jump to bottom

Added support for brotli ('br') content-encoding #403

Closed
wants to merge 2 commits into from
Closed

Added support for brotli ('br') content-encoding #403

wants to merge 2 commits into from

Conversation

danielgindi
Copy link

@danielgindi danielgindi commented Jul 10, 2020 •
edited

@danielgindi danielgindi force-pushed the feature/brotli branch 6 times, most recently from 2015f22 to 5c8d15f Compare July 10, 2020 06:01
@danielgindi danielgindi mentioned this pull request Jul 10, 2020
Open
dougwilson
dougwilson suggested changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

The docs need to be updated to add the new encoding that is inflated and explain how the users know if it will be available or not.

lib/read.js Outdated Show resolved Hide resolved
lib/read.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/json.js Outdated Show resolved Hide resolved
test/raw.js Outdated Show resolved Hide resolved
test/urlencoded.js Outdated Show resolved Hide resolved
test/text.js Outdated Show resolved Hide resolved
test/json.js Outdated Show resolved Hide resolved
test/raw.js Outdated Show resolved Hide resolved
test/text.js Outdated Show resolved Hide resolved
@danielgindi danielgindi force-pushed the feature/brotli branch 3 times, most recently from 8a297bb to a6dd921 Compare July 10, 2020 13:34
dougwilson
dougwilson suggested changes Jul 10, 2020
View reviewed changes
README.md Outdated
@@ -16,6 +16,8 @@ before trusting. For example, `req.body.foo.toString()` may fail in multiple
ways, for example the `foo` property may not be there or may not be a string,
and `toString` may not be a function and instead a string or other user input.

**Note** Brotli provides better and faster compression then gzip or deflate, but is supported only since Node.js versions v11.7.0 and v10.16.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please hard-wrap this line to match the width of the rest of the lines. In addition, since this module only ever decompresses, is this note about compression relevant? I would think that the information about version support should be along with the br support docs, otherwise users may not see it.

Copy link
Author

Choose a reason for hiding this comment

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

I altered the note

Copy link
Contributor

Choose a reason for hiding this comment

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

Please stop marking this as resolved when you didn't address all my comments or respond back with why you don't think you should address some part to have a discussion on it.

Copy link
Author

Choose a reason for hiding this comment

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

I did hard wrap this when resolving the original comment, and now this line does not even exist 😂
This is why it was hidden. It's marked as "outdated" automatically

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that the information about version support should be along with the br support docs, otherwise users may not see it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OMG please STOP
image

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/read.js Outdated Show resolved Hide resolved
test/json.js Outdated Show resolved Hide resolved
@danielgindi danielgindi force-pushed the feature/brotli branch 3 times, most recently from b995bfe to 6c80c9e Compare July 10, 2020 15:47
dougwilson
dougwilson suggested changes Jul 10, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@danielgindi
Copy link
Author

@dougwilson Are we good to go? ;-)

@dougwilson
Copy link
Contributor

No, it seems like since you keep force pushing it is closing out unresolved comments I made. Please make sure you have responded to all my outstanding requests and don't resolve them before you do or without commenting on them first :)

@dougwilson
Copy link
Contributor

I just tried to unresolve a few but they still do not show. I will plan to re-review and remake those comments again next week. Have a good weekend.

@danielgindi
Copy link
Author

I remember resolving them all, but please add any outstanding comments...

@dougwilson
Copy link
Contributor

I was just able to get at least one of the unresolved ones to re-show up as unresolved again above.

@danielgindi
Copy link
Author

I was just able to get at least one of the unresolved ones to re-show up as unresolved again above.

I addressed them all now. Improved the docs a bit.

dougwilson
dougwilson suggested changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

There is still an unresolved comment.

@danielgindi
Copy link
Author

There is still an unresolved comment.

It shows me that you "requested changes" but does not show any changes that were requested!

@dougwilson
Copy link
Contributor

Hm, weird. Here is a link to it: #403 (comment)

@dougwilson dougwilson closed this Jul 13, 2020
@dougwilson
Copy link
Contributor

It is clear this PR is quite a mess at this point, and things are getting resolved that are not resolved. Please open this PR again so we can restart the review process 🙏

@dougwilson
Copy link
Contributor

Edit: to clarify, I closed this because it seems like the comments are getting resolved likely automatically, and so it's hard to keep track of what is and is not resolved at this point. I think a fresh PR and a new review with this current iteration of code would help a lot in getting this landed is all. If there was a way in GitHub for me to just erase all comments above or something to "reset" a PR that would be awesome, but unfortunately there is no such feature... :(

@danielgindi danielgindi mentioned this pull request Jul 13, 2020
Open
@danielgindi
Copy link
Author

Edit: to clarify, I closed this because it seems like the comments are getting resolved likely automatically, and so it's hard to keep track of what is and is not resolved at this point. I think a fresh PR and a new review with this current iteration of code would help a lot in getting this landed is all. If there was a way in GitHub for me to just erase all comments above or something to "reset" a PR that would be awesome, but unfortunately there is no such feature... :(

Okay, made a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougwilson dougwilson dougwilson requested changes

Assignees
No one assigned
Labels
enhancement pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@danielgindi @dougwilson