-
Notifications
You must be signed in to change notification settings - Fork 563
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
Update llhttp #691
Update llhttp #691
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,79 @@ | ||
# Developer's Certificate of Origin 1.1 | ||
# Contributing to Undici | ||
|
||
* [Guides](#guides) | ||
* [Update `llhttp`](#update-llhttp) | ||
* [Developer's Certificate of Origin 1.1](#developers-certificate-of-origin) | ||
* [Moderation Policy](#moderation-policy) | ||
|
||
<a id="guides"></a> | ||
## Guides | ||
|
||
<a id="update-llhttp"></a> | ||
### Update `llhttp` | ||
|
||
The HTTP parser used by `undici` is a WebAssembly build of [`llhttp`](https://github.com/nodejs/llhttp). | ||
|
||
While the project itself provides a way to compile targeting WebAssembly, at the moment we embed the sources | ||
directly and compile the module in `undici`. | ||
|
||
The `deps/llhttp/include` folder contains the C header files, while the `deps/llhttp/src` folder contains | ||
the C source files needed to compile the module. | ||
|
||
The `lib/llhttp` folder contains the `.js` transpiled assets required to implement a parser. | ||
|
||
The following are the steps required to perform an update. | ||
|
||
#### Clone the [llhttp](https://github.com/nodejs/llhttp) project | ||
|
||
```bash | ||
git clone git@github.com:nodejs/llhttp.git | ||
|
||
cd llhttp | ||
``` | ||
#### Checkout a `llhttp` release | ||
|
||
```bash | ||
git checkout <tag> | ||
``` | ||
|
||
#### Install the `llhttp` dependencies | ||
|
||
```bash | ||
npm i | ||
``` | ||
|
||
#### Run the wasm build script | ||
|
||
```bash | ||
npm run build-wasm | ||
``` | ||
|
||
#### Copy the sources to `undici` | ||
|
||
```bash | ||
cp build/wasm/*.js <your-path-to-undici>/lib/llhttp/ | ||
|
||
cp build/wasm/*.js.map <your-path-to-undici>/lib/llhttp/ | ||
|
||
cp build/wasm/*.d.ts <your-path-to-undici>/lib/llhttp/ | ||
|
||
cp src/native/api.c src/native/http.c build/c/llhttp.c <your-path-to-undici>/deps/llhttp/src/ | ||
|
||
cp src/native/api.h build/llhttp.h <your-path-to-undici>/deps/llhttp/include/ | ||
``` | ||
|
||
#### Build the WebAssembly module in `undici` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also this would not be needed of we didn't embed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid embedding as of now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can. We don't currently modify the source files. IMHO is a cleaner approach, as long as we don't have to customize it. One thing to consider is the direction the But regarding now, we would just have to run |
||
|
||
> This requires [docker](https://www.docker.com/) installed on your machine. | ||
|
||
```bash | ||
cd <your-path-to-undici> | ||
|
||
npm run build:wasm | ||
``` | ||
|
||
<a id="developers-certificate-of-origin"></a> | ||
## Developer's Certificate of Origin 1.1 | ||
|
||
By making a contribution to this project, I certify that: | ||
|
||
|
@@ -24,7 +99,8 @@ By making a contribution to this project, I certify that: | |
maintained indefinitely and may be redistributed consistent with | ||
this project or the open source license(s) involved. | ||
|
||
## Moderation Policy | ||
<a id="moderation-policy"></a> | ||
### Moderation Policy | ||
|
||
The [Node.js Moderation Policy] applies to this project. | ||
|
||
|
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.
All these steps could be condensed to:
If we remove the embedding and use the
llhttp
project build. Just wanted to say that's an option if this is too clumsy.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.
This guide should be revisited if/when we have to modify
lhttp
sources for whatever reason.