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

fix(node:http) Export validateHeaderName and validateHeaderValue functions #22616

Merged
merged 5 commits into from
Mar 10, 2024
Merged

Conversation

mash-graz
Copy link
Contributor

@mash-graz mash-graz commented Feb 28, 2024

Modify _http_outgoing.ts to support the extended signature of validateHeaderName() used since node v19.5.0/v18.14.0 by adding the label parameter. (see: https://nodejs.org/api/http.html#httpvalidateheadernamename-label)

Making both validation functions accessible as public exports of node:http

Fixes: #22614

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

@mash-graz mash-graz changed the title fix(node:http) Export validateHeaderName and validateHeaderValue fix(node:http) Export validateHeaderName and validateHeaderValue functions Feb 28, 2024
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting check fails.

Also, could you add "test-http-header-validators.js" string to tests/node_compat/config.jsonc line 369. I think that should enable the tests for these.

ext/node/polyfills/_http_outgoing.ts Outdated Show resolved Hide resolved
@aapoalas
Copy link
Collaborator

aapoalas commented Mar 5, 2024

Here's a good example of what enabling a node test entails: https://github.com/denoland/deno/pull/22489/files#diff-66a13180ca921c746469f8f0bf6ec0cafab925839d65fdca6379866c4bb7be40

Note this part:

NOTE: This file should not be manually edited. Please edit tests/node_compat/config.json and run deno task setup in tools/node_compat dir instead.

@mash-graz
Copy link
Contributor Author

The fmt issues should be now fixed and a relevant node_compat test is also activated.

Please review again.

@mash-graz mash-graz requested a review from aapoalas March 6, 2024 09:13
@mash-graz
Copy link
Contributor Author

mash-graz commented Mar 7, 2024

Please take a look at this more or less trivial modifications and merge them!

The included changes are so simple, that I didn't open a new branch when I opened this PR, but in the meantime I prepared a lot of other more extensive patches for deno, which I can't simply transfer to github, because this PR and it's fixed source branch location are blocking all my further activities... :(

sorry!

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks, good fix.

@bartlomieju bartlomieju enabled auto-merge (squash) March 10, 2024 22:12
@bartlomieju bartlomieju merged commit 16dbbfa into denoland:main Mar 10, 2024
17 checks passed
nathanwhit pushed a commit that referenced this pull request Mar 14, 2024
…functions (#22616)

Modify `_http_outgoing.ts` to support the extended signature of
`validateHeaderName()` used since node v19.5.0/v18.14.0 by adding the
`label` parameter. (see:
https://nodejs.org/api/http.html#httpvalidateheadernamename-label)

Making both validation functions accessible as public exports of
`node:http`

Fixes: #22614
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.

missing reexport of validateHeaderName and validateHeaderValue in node:http
4 participants