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

Use headersSent instead of _header #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use headersSent instead of _header #129

wants to merge 1 commit into from

Conversation

maritz
Copy link

@maritz maritz commented Jan 10, 2018 •
edited

As discussed in #127 this PR now includes the changes that do break node v0.8 and has tests.

I did not remove the travis config for v0.8 since I'm not sure that's something that should be in a PR.

@maritz maritz mentioned this pull request Jan 10, 2018
Closed
@maritz
Copy link
Author

maritz commented Jan 10, 2018

Hm, I just noticed that I branched from my other changes. Should I remove those changes from this PR?

@maritz maritz changed the title Use headersSent instead of _header WIP: Use headersSent instead of _header Jan 10, 2018
@dougwilson dougwilson self-assigned this Jan 15, 2018
@dougwilson dougwilson added the pr label Jan 15, 2018
@dougwilson
Copy link
Contributor

Hi @maritz I can try to cut them out myself during the merge, but if you can remove them, that may be good especially in case something else changes (either in the other PR or between that PR merging and the major release to include this change).

@dougwilson dougwilson added this to the 2.0 milestone Jan 15, 2018
@dougwilson dougwilson force-pushed the master branch 3 times, most recently from d7bb81b to cd957aa Compare May 30, 2018 04:09
@maritz maritz force-pushed the change/use_headersSent_instead_of__header branch from de4f632 to 24a6740 Compare September 27, 2018 10:50
@maritz
Copy link
Author

maritz commented Sep 27, 2018

@dougwilson I rebased and cut out the extra commits.

@maritz maritz changed the title WIP: Use headersSent instead of _header Use headersSent instead of _header Sep 27, 2018
@curbengh curbengh mentioned this pull request Nov 2, 2018
Open
@Icehunter Icehunter mentioned this pull request Mar 2, 2020
Open
lamweili
lamweili suggested changes Aug 4, 2022
View reviewed changes
Copy link

@lamweili lamweili left a comment

Choose a reason for hiding this comment

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

Would this change be better?
It will be backwards compatible with Node.js v0.8 and not incur a semver major.

index.js
@@ -80,7 +80,7 @@ function compression (options) {
return false
}

if (!this._header) {
if (!this.headersSent) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (!this.headersSent) {
if (typeof this.headersSent === 'boolean' ? !this.headersSent : !this._header) {

index.js
@@ -94,7 +94,7 @@ function compression (options) {
return false
}

if (!this._header) {
if (!this.headersSent) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (!this.headersSent) {
if (typeof this.headersSent === 'boolean' ? !this.headersSent : !this._header) {

@lamweili
Copy link

lamweili commented Aug 4, 2022

For background information regarding res._header and res.headersSent equivalence:
https://github.com/nodejs/node/blob/93e0bf9abf350637d772bcce14a5d9527b733300/lib/_http_outgoing.js#L741-L746

We can also add test cases to assert the equivalence (if really required).
The code snippet is available @ expressjs/session@35880d6.

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

@lamweili lamweili lamweili requested changes

Assignees

@dougwilson dougwilson

Labels
enhancement needs rebase pr
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@maritz @dougwilson @lamweili