feat(core): lower case http headers, based on env variable. Fixes #311

[alexanderbartels]

PR Type

Lower Case HTTP Headers behind a Feature Flag as discussed in #311

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #311

What is the new behavior?

all http headers are lower cased if the env flag is defined.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Just a first draft. as the flag is based on the env flag and it seems to be a good idea to cache the value for performance reason i had no idea how to test with and without the flag.
I can continue after some general feedback.

[alexanderbartels]

Change is only validated using the automated test cases.
I hadn't the time to try the changes in my own project..

[codecov]

Codecov Report

Merging #318 (9a8aa25) into master (cbdffda) will increase coverage by 0.31%.
The diff coverage is 96.51%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   95.41%   95.72%   +0.31%     
==========================================
  Files         157      164       +7     
  Lines        2769     3137     +368     
  Branches      366      431      +65     
==========================================
+ Hits         2642     3003     +361     
- Misses        123      129       +6     
- Partials        4        5       +1     

Impacted Files	Coverage Δ
packages/core/src/context/context.hook.ts	100.00% <ø> (ø)
...ackages/core/src/effects/effectsContext.factory.ts	100.00% <ø> (ø)
packages/core/src/logger/logger.interface.ts	100.00% <ø> (ø)
packages/messaging/src/reply/reply.ts	100.00% <ø> (ø)
...rc/transport/strategies/amqp.strategy.interface.ts	81.81% <ø> (ø)
.../testing/src/testBed/http/http.testBed.response.ts	75.00% <40.00%> (-13.24%)	⬇️
packages/core/src/+internal/utils/string.util.ts	92.00% <66.66%> (-8.00%)	⬇️
packages/core/src/+internal/utils/any.util.ts	84.61% <71.42%> (+1.28%)	⬆️
...ore/src/http/response/http.responseBody.factory.ts	80.95% <73.33%> (-19.05%)	⬇️
...ssaging/src/transport/strategies/local.strategy.ts	92.15% <83.33%> (+4.92%)	⬆️
... and 74 more

---

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 bcdc101...9a8aa25. Read the comment docs.

[JozefFlakus]

Thanks for creating this PR. I'll try to check it tomorrow! 🙌

[JozefFlakus]

@alexanderbartels Finally found some time for reviewing it, sorry for the late response. Before I'll dig more into the details, I have one general question. Why do you think it is needed to memoize this operation. Reading from process.env is a straightforward operation since it is just a plain object 🤔

[JozefFlakus]

I would extract this process to separate function, eg. #normalizeHeaders - then you can use simple ternary check. If we care here about the performance impact then it can stay as it is, but if not it would be better to use reduce instead of forEach. In typical case the total number of outgoing headers won't reach more than 10.

[alexanderbartels]

I have one general question. Why do you think it is needed to memoize this operation. Reading from process.env is a straightforward operation since it is just a plain object 🤔

not sure if its still a problem but here are some references:
https://stackoverflow.com/questions/51138302/is-process-env-node-env-still-slow-in-node-8

cite from https://expressjs.com/en/advanced/best-practice-performance.html

If you need to write environment-specific code, you can check the value of NODE_ENV with process.env.NODE_ENV. Be aware that checking the value of any environment variable incurs a performance penalty, and so should be done sparingly.

and from node issues: nodejs/node#3104
issue was closed with this comment:

Doesn't seem worthwhile. Closing, please cache it yourself if need be. :)

As we hope to get a lot of traffic it might be still a good idea to cache it. But to be really sure if it is still an issue, a performance test would be needed.

[JozefFlakus]

@alexanderbartels Ok, you convinced me! 🙃

[JozefFlakus]

@alexanderbartels regarding testing - you can write simple integration/E2E scenarios directly inside @integration module. Do you need more guidance here?

[JozefFlakus]

@alexanderbartels any updates? If not I'll take it over. I'm planning to introduce it in next major version 4.0

[JozefFlakus]

@alexanderbartels could you sync your PR with next branch? I really would like to include this improvement with the upcoming major release but for that I need to have it up-to date. You can also add some additional permissions to your already existing PR in case I would like to take over the work.

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

[JozefFlakus]

Duplicates: #349