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

(327) Fix - Custom headers of one component can appear to another. #328

Conversation

i-b-dimitrov
Copy link
Contributor

@i-b-dimitrov i-b-dimitrov commented Dec 9, 2016

The problem was introduced with #326 -
In get-component.js the declaration of responseHeaders variable happens in a more
global scope than it should be. For this reason every invocation of renderer
function sees the changes in the headers made by previously requested
components.
The fix is to just move the declaration one scope inner.

  • Added a unit test to validate the fix - it tries to load two components
    sequentially. The first one does provide custom headers while the second
    one - doesn't. Then we check and expect to see no custom headers
    in the result for the second one.

The problem was introduced with opencomponents#326 -
In get-component.js the declaration of responseHeaders variable happens in a
more global scope than it should be. For this reason every invocation of
renderer function sees the changes in the headers made by previously requested
components.
The fix is to just move the declaration one scope inner.

- Added a unit test to validate the fix - it tries to load two components
subsequently. The first one does provide custom headers while the second
one - don't. Then we check and expect to see there are no custom headers
in the result for the second one.
@matteofigus
Copy link
Member

Excellent, thanks @ivan-dimitrov-skyscanner

@matteofigus matteofigus merged commit 21c649a into opencomponents:master Dec 9, 2016
@i-b-dimitrov i-b-dimitrov deleted the 327-Fix-For-Custom-Cache-Headers-Inheritance branch December 9, 2016 14:52
@matteofigus
Copy link
Member

This fixed #327

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants