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.

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

fix: allow stream protocols to return headers with multiple values #14887

Merged
merged 4 commits into from Oct 25, 2018

Conversation

Projects
None yet
4 participants
@ibash
Copy link
Contributor

commented Sep 30, 2018

Description of Change

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

cc @sofianguy

Checklist
Release Notes

Notes: Fixed returning headers with multiple values for stream protocols.

@ibash ibash requested a review from as a code owner Sep 30, 2018

@welcome

This comment has been minimized.

Copy link

commented Sep 30, 2018

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@ibash

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

@MarshallOfSound updated! did you want me to dedupe the method as well?

@ibash

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

@ckerr
Copy link
Member

left a comment

It's great to see first time submitters come up with solid code and tests. Thanks for the PR!

Show resolved Hide resolved atom/common/native_mate_converters/net_converter.cc Outdated
Show resolved Hide resolved atom/common/native_mate_converters/net_converter.cc Outdated
@ibash

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2018

@ckerr back to you!

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

@ibash you have a consistently failing test here, once CI is green we can take another look at this

@ibash

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

@MarshallOfSound fixed! (not sure how I missed that before, sorry about that!)

@ckerr

ckerr approved these changes Oct 23, 2018

Copy link
Member

left a comment

These CI failures seem unrelated to the PR itself, e.g. cannot open display: :99.0.

I'd prefer to see CI green and have raised an issue in the Slack channel, but am OK with this landing regardless

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

@ckerr I'm rerunning the linux CI from the beginning, something probably broke in the middle and is influencing the partial reruns.

🔗 https://circleci.com/workflow-run/e97ae132-c6fc-477f-a971-79d64643f753

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@ibash can you rebase this PR with the latest from master? The CI tests are failing due to an issue that was resolved in master.

ibash added some commits Sep 30, 2018

fix: allow stream protocols to return headers with multiple values
This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778
Cleanup header conversion
1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)
Fix broken test
Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

@ibash ibash force-pushed the ibash:fix_header_conversion branch from b4d8c54 to 463fbc3 Oct 23, 2018

@ibash

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

@ckerr

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

@MarshallOfSound OK to merge?

@MarshallOfSound MarshallOfSound merged commit 3b6f0d8 into electron:master Oct 25, 2018

21 of 22 checks passed

ci/circleci: mas-testing-tests Your tests failed on CircleCI
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20181023.35 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Oct 25, 2018

Release Notes Persisted

Fixed returning headers with multiple values for stream protocols.

@welcome

This comment has been minimized.

Copy link

commented Oct 25, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@ibash

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2018

@MarshallOfSound any opposition to backporting this to 3 and 4?

ibash added a commit to ibash/electron that referenced this pull request Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

ibash added a commit to ibash/electron that referenced this pull request Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

ibash added a commit to ibash/electron that referenced this pull request Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

notes: Fixes a bug that prevented stream protocols from returning headers with multiple values.

Fixes electron#14778

ibash added a commit to ibash/electron that referenced this pull request Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

notes: Fixed returning headers with multiple values for stream protocols.

Fixes electron#14778

miniak added a commit that referenced this pull request May 1, 2019

fix: allow stream protocols to return headers with multiple values (#…
…14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.
@trop

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

A maintainer has manually backported this PR to "4-1-x", please check out #18094

2 similar comments
@trop

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

A maintainer has manually backported this PR to "4-1-x", please check out #18094

@trop

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

A maintainer has manually backported this PR to "4-1-x", please check out #18094

@trop trop bot added the in-flight/4-1-x label May 1, 2019

MarshallOfSound added a commit that referenced this pull request May 1, 2019

fix: allow stream protocols to return headers with multiple values (#…
…14887) (#18094)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.