Allow components to support the req component parameter in Typescript
#67
Conversation
|
@thibmeu I'm not quite sure how to resolve these build errors - I ran |
AkshatM
force-pushed
the
akshat/update-typescript-to-support-req
branch
from
e73aa76 to
e5eda65
Compare
thibmeu
left a comment
thibmeu
left a comment
There was a problem hiding this comment.
The change is correct, but sacrifices a lot of the developer experience.
'@authority' becomes{name: '@authority'}``- Responses as parameter becomes
{response: Response}
As a follow up, it could also be interesting to have some performance measurements.
For tsup, you can add the following tsconfig.json under packages/http-message-sig, similar to the one present in web-bot-auth
For building, as long as you do it from the root, it should work. It seems web-bot-auth package needs update based on your API changes in http-message-sig. Ideally, the package would not need any type update (or more limited, based on the DX comments)
{
"compilerOptions": {
"target": "es2021",
"module": "es2022",
"moduleResolution": "bundler",
"skipLibCheck": true
}
}| parameters?: ComponentParameters; | ||
| } | ||
|
|
||
| export type ComponentParameters = Map<string, string | boolean>; |
There was a problem hiding this comment.
Does this have to be a map, or are you looking for Record<string, string|boolean>? https://www.typescriptlang.org/docs/handbook/utility-types.html#recordkeys-type
There was a problem hiding this comment.
A map is what the structured-header package uses to represent component parameters. I found it quite useful, since it means you automatically have access to .has(), .get(), .set() + guarantees on iteration order, and supports arbitrary parameters - it feels like the "correct" interface.
What is the advantage of a record type here? That is, what are we trying to preserve?
AkshatM
force-pushed
the
akshat/update-typescript-to-support-req
branch
2 times, most recently
from
2aba1db to
31b586c
Compare
|
|
thibmeu
left a comment
thibmeu
left a comment
There was a problem hiding this comment.
added some nits/minor fixes
in term of bundle size, it is bounded.
before the change
CJS dist/index.js 11.53 KB
CJS ⚡️ Build success in 20ms
ESM dist/index.mjs 10.31 KB
ESM ⚡️ Build success in 21ms
DTS Build start
DTS ⚡️ Build success in 463ms
DTS dist/index.d.ts 3.98 KB
DTS dist/index.d.mts 3.98 KB
after the change
CJS dist/index.js 12.67 KB
CJS ⚡️ Build success in 22ms
ESM dist/index.mjs 11.22 KB
ESM ⚡️ Build success in 22ms
DTS Build start
DTS ⚡️ Build success in 454ms
DTS dist/index.d.ts 4.77 KB
DTS dist/index.d.mts 4.77 KB
| await signatureHeaders(message, { | ||
| signer, | ||
| components, | ||
| RESPONSE_COMPONENTS, |
There was a problem hiding this comment.
| RESPONSE_COMPONENTS, | |
| components: RESPONSE_COMPONENTS, |
| dictionary = parseDictionary(header.toString()); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Invalid ${name} header; failed to parse as RFC 8941 dictionary: ${error.toString()}` |
There was a problem hiding this comment.
| `Invalid ${name} header; failed to parse as RFC 8941 dictionary: ${error.toString()}` | |
| `Invalid ${name} header; failed to parse as RFC 8941 dictionary: ${(error as Error).message}` |
| if (!(message as ResponseLike).status) | ||
| throw new Error(`${component} is only valid for responses`); | ||
| return (message as ResponseLike).status.toString(); | ||
| case "@query-params": |
There was a problem hiding this comment.
keep the error consistent
| case "@query-params": | |
| case "@query-params": | |
| throw new Error(`${component} is not implemented yet`); |
`http-message-sig` package is currently not compliant with the spec - in particular, it returns `@authority` as a derived component in the response, when in reality it should be returning `"@authority";req`. This makes it impossible to write a compliant HTTP message directory, for instance. To support this, I've made several changes: 1. I've defined a new type called `ComponentParameters`, which can store arbitrary parameters attached to components. This allows us to expose `req`, `bs`, `sf`, etc. and even `key`. I've added it as a variant of `Component`. 2. I've replaced the handwritten parser with `structured-headers` library, which does the parsing for us. I've done my best to preserve the error cases we had in the past, but I've removed cases that assumed an unknown parameter meant invalid parsing - unknown list parameters are fine to keep and do not imply invalid parsing. 3. I've defined a new type called `ResponseRequestPair`, which bundles both request and response together, and written a resolver that inspects whether or not a component needs the pair, or can make do with just the raw message.
AkshatM
force-pushed
the
akshat/update-typescript-to-support-req
branch
from
31b586c to
b3fd834
Compare
2 participants
http-message-sigpackage is currently not compliant with the spec - in particular, it returns@authorityas a derived component in the response, when in reality it should be returning"@authority";req.This makes it impossible to write a compliant HTTP message directory, for instance.
To support this, I've made several changes:
I've defined a new type called
ComponentParameters, which can store arbitrary parameters attached to components. This allows us to exposereq,bs,sf, etc. and evenkey.I've replaced the handwritten parser with
structured-headerslibrary, which does the parsing for us. I've done my best to preserve the error cases we had in the past, but I've removed cases that assumed an unknown parameter meant invalid parsing - unknown list parameters are fine to keep and do not imply invalid parsing.I've altered all the signing and verifying functions to accept either a
RequestLikeor a pair ofResponseLikeand an optionalRequestLike, instead of a rawResponseLike. This allows all signers and verifiers to have access to the request.