Chain responses in middleware.

[mjarosch]

• This PR follows the Contribution Guide
• Changelog updated

Description / Motivation

By chaining the responses in rewrites, we can access headers set in earlier middleware.

Testing Details

• Unit Test Added
• Manual Test/Other - I have implemented similar changes in our Sitecore solution. I don't have a personal Sitecore instance to fully test them on at this time.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

[mjarosch]

By making this change, middleware should favor headers for passing data. Cookies are in the header and update, but don't appear in the 'cookies' property, so would be reserved for passing data to future requests.

[mjarosch]

Sorry for all the yarn.lock commits.

[art-alexeyenko]

@mjarosch It took us a while to get to this, sorry.
This is great work and an amazing code cleanup. I might need to copy the changes into a separate branch for some cleanup and conflict resolution - or if you got time, let me know if you can adjust personalize-middleware and remove yarn.lock from changes in this PR (there are no new dependencies - so yarn.lock does not need to be updated)

I did want to ask about reading site from header. We add sc_site cookie to ensure it's used in subsequent page requests - this is a compatibility measure for Sitecore, since it reads site name from the sc_site too. We also read the value from response object:
res?.cookies.get(this.SITE_SYMBOL)?.value
What is the benefit of reading site from header? You left this comment

// Note: Cookies will be added to the header, but will not appear in the "cookies" property
 //       of later middleware calls. Values for later middleware should be passed in the header.

Do you mean that cookies may be absent during middleware execution of other requests or that follow-up middleware plugins have no access to cookies?

[mjarosch]

@art-alexeyenko Yes, I will update to remove the yarn.lock file and resolve the conflicts in personalize.

Do you mean that cookies may be absent during middleware execution of other requests or that follow-up middleware plugins have no access to cookies?

I mean follow-up middleware plugins won't have access to the cookie. The cookie is still set on the client, and future requests will see it.

[art-alexeyenko]

@mjarosch

I mean follow-up middleware plugins won't have access to the cookie. The cookie is still set on the client, and future requests will see it.

That's not what I'm seeing when testing locally - but perhaps I miss some scenarios. SITE_SYMBOL cookie is set in multisite plugin, and if you check the value in the follow up plugin like
console.log(res?.cookies.get(this.SITE_SYMBOL)?.value);
The value will be available, on server. I also tested this in incgonito to ensure I'm not reading cookies from previous requests.

[a60915]

@art-alexeyenko I could have sworn, when I originally did this, that it wasn't there. But I validated on my original implementation and they are accessible. Should I remove the comment?

[art-alexeyenko]

@mjarosch perhaps this PR can just be wound down to code cleanup? There's probably no reason to duplicate site in headers and cookies - but it's good to avoid code duplication.

[mjarosch]

@art-alexeyenko the chaining makes it so following middleware don’t need to re-set cookies. So I would consider that more than code cleanup.

I can remove adding the site name to the headers. I think it’s nice to pass things in a consistent place and not everything should be set in a cookie. So I think you could justify duplicating the site in this case.

I defer to your judgement on this.

[art-alexeyenko]

@mjarosch I'm still not sure the site should be duplicated in two places. The cookie can be used for both Sitecore instance to be consumed by, and by middleware plugins. Header will just repeat what is already set.
Could you cleanup the part around setting the header please, and then we'll merge this into the product.

[mjarosch]

@art-alexeyenko I have updated it to remove storing the site in the header.

[art-alexeyenko]

@mjarosch thank you once again!
I left a couple small comments and pinged a dev from SXA team to give a thumbs up on redirects changes, as it's their domain. We should be able to merge once there's a green light.