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[bot]]

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%)	:arrow_down:
packages/core/src/+internal/utils/string.util.ts	92.00% <66.66%> (-8.00%)	:arrow_down:
packages/core/src/+internal/utils/any.util.ts	84.61% <71.42%> (+1.28%)	:arrow_up:
...ore/src/http/response/http.responseBody.factory.ts	80.95% <73.33%> (-19.05%)	:arrow_down:
...ssaging/src/transport/strategies/local.strategy.ts	92.15% <83.33%> (+4.92%)	:arrow_up:
... 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 🤔

[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: https://github.com/nodejs/node/issues/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