-
Notifications
You must be signed in to change notification settings - Fork 49
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
migrate to class #271
base: master
Are you sure you want to change the base?
migrate to class #271
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎
// TODO change this to use the header getter | ||
const regex = new RegExp('\\r\\n' + name + ': ([^\\r]*)\\r\\n') | ||
const [, value] = this._header.match(regex) || [] | ||
// const value = this.getHeader(name) | ||
if (value) { | ||
this._lightMyRequest.headers[name.toLowerCase()] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will the unit tests pass if you use the header getter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean, this function is same as original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not pass, these headers 'Data', 'Connection', 'Transfer-Encoding'
do not exist
@jsumners, Can you explain your negative attitude? |
@jsumners I'm not mentioning bun in this PR (since not all issues are resolved) |
migrate to class syntax (Chain, request, response)
copied class from #252
but without changing the logic in
lib/do-inject.js
(doInject
) (to resolve Test Fastify Integration)Checklist
npm run test
andnpm run benchmark
and the Code of conduct