Skip to content

Feature: Add 'mergeCookies' options #1375

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feature: Add 'mergeCookies' options #1375

wants to merge 2 commits into from

Conversation

RidgeA
Copy link

@RidgeA RidgeA commented Aug 28, 2019

Replace of #1318

@RidgeA RidgeA mentioned this pull request Aug 28, 2019
Closed
@codecov-io
Copy link

codecov-io commented Aug 28, 2019 •
edited
Loading

Codecov Report

Merging #1375 into master will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
+ Coverage   92.35%   92.61%   +0.25%     
==========================================
  Files           6        6              
  Lines         314      325      +11     
==========================================
+ Hits          290      301      +11     
  Misses         24       24
Impacted Files Coverage Δ
lib/http-proxy.js 100% <ø> (ø) ⬆️
lib/http-proxy/passes/web-outgoing.js 100% <100%> (ø) ⬆️
lib/http-proxy/common.js 97.36% <100%> (+0.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d056241...3fcde39. Read the comment docs.

@RidgeA RidgeA force-pushed the feature/add_merge_cookie_options branch from 1fc1ef4 to 5fa7a1c Compare August 28, 2019 08:26
@RidgeA RidgeA force-pushed the feature/add_merge_cookie_options branch from 5fa7a1c to 11480ab Compare August 28, 2019 08:30
jsmylnycky
jsmylnycky suggested changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@jsmylnycky jsmylnycky left a comment

Choose a reason for hiding this comment

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

Besides the couple spots to clean up the quotations, looks good to me. Go ahead and fix those up, and I'll put a note to review this PR for one of the next couple releases.

Thanks 😄

lib/http-proxy/passes/web-outgoing.js Outdated
@@ -95,6 +96,9 @@ module.exports = { // <--
if (rewriteCookiePathConfig && key.toLowerCase() === 'set-cookie') {
header = common.rewriteCookieProperty(header, rewriteCookiePathConfig, 'path');
}
if (mergeCookiesConfig && key.toLowerCase() === 'set-cookie') {
header = common.mergeSetCookie(res.getHeader("set-cookie"), header)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Replace the " with '

test/lib-http-proxy-passes-web-outgoing-test.js Outdated

this.proxyRes.headers = Object.assign({}, this.proxyRes.headers, {'set-cookie': 'hello1; domain=my.domain; path=/'});

this.res.setHeader("set-cookie", 'hello; domain=my.domain; path=/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Replace the " with '

@jsmylnycky jsmylnycky added the Will merge PRs that will be merged in the order that is most logical for the project. label Aug 28, 2019
@jsmylnycky jsmylnycky changed the title Feature: Add 'megreCookies' options Feature: Add 'mergeCookies' options Aug 29, 2019
@mciocan
Copy link

mciocan commented Sep 30, 2020

Hello,

Any updates for this feature merge?

Thanks

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.61%. Comparing base (d056241) to head (3fcde39).
Report is 5 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
+ Coverage   92.35%   92.61%   +0.25%     
==========================================
  Files           6        6              
  Lines         314      325      +11     
==========================================
+ Hits          290      301      +11     
  Misses         24       24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@jsmylnycky jsmylnycky jsmylnycky approved these changes

Assignees
No one assigned
Labels
Will merge PRs that will be merged in the order that is most logical for the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RidgeA @codecov-io @mciocan @codecov-commenter @jsmylnycky