nginx / njs

A subset of JavaScript language to use in nginx
http://nginx.org/en/docs/njs/
BSD 2-Clause "Simplified" License
1.27k stars 156 forks source link

Support Web APIs. #299

Open hongzhidao opened 4 years ago

hongzhidao commented 4 years ago

@xeioex @drsm

Web APIs is an alternative usage. https://developer.mozilla.org/en-US/docs/Web/API/Response/Response It will behave in njs like below.

function hello(request) {
      return new Response('hello');
}

function subrequest(request) {
      var sr = new Request('/sub');
      sr.fetch()
      .then(response) {
          return response;
      };
}

In the beginning, njs has individual response object. foo(req, res). After that, res is merged into req, so we have to rename some methods like headersIn, headersOut. If we introduce these things, Request, Response, Headers, Body, I believe it can be easier to use, and we just implement the standard methods with them. The second change is that the subrequest is also easier to use. Indeed, users need not know what subrequest is, they just want to send a request. (But the implementation will be upon on nginx subrequest). Still, further, it can be welcome after async, await are introduced.

Highly welcome to your suggestions. I'd like to try it without changing current usage by put them in precontent phase.

I'm still not familiar with async, await, Promise, it's good to have a better example of how to write with subrequest below. @drsm

BTW, do you know whether the above objects are standard belong to js? I'm wondering which software has already implemented them, where are they used?

xeioex commented 4 years ago

@hongzhidao

BTW, do you know whether the above objects are standard belong to js?

There are a ton of web standards. ECMAScript® 2020 Language Specification (JavaScript) is just one of the many. DOM is another example. They are related but otherwise orthogonal.

I'm wondering which software has already implemented them, where are they used?

https://developer.mozilla.org/en-US/docs/Web/API/Response/Response#Browser_compatibility

BTW, every feature is marked as "Experimental. Expect behavior to change in the future."

So we probably should wait a bit until dust is settled.

I'm still not familiar with async, await, Promise, it's good to have a better example of how to write with subrequest below.

Take a look at an example.

hongzhidao commented 4 years ago

@xeioex @drsm @lexborisov

Take a look at an example of generator.

function* genFunc() {
    yield 'a';
    yield 'b';
    yield 'c';
}

genObj = genFunc();

console.log(genObj.next());
console.log(genObj.next());
console.log(genObj.next());
console.log(genObj.next());

We've talked about the usage of lua-module, that developers can write async-operations in sync way.

local res = ngx.location.capture("/some_other_location")
if res then
  ngx.say("status: ", res.status)
  ngx.say("body:")
  ngx.print(res.body)
end

Lua has a mechanism called coroutine. See an example.

thread2 = coroutine.create(function()
  for n = 1, 5 do
    print("honk "..n)
    coroutine.yield()
  end
end)

coroutine.resume(thread2)
-->> honk 1
coroutine.resume(thread2)
-->> honk 2

Actually, each request is wrapped as a coroutine function, so lua-module can do the async things like capture.

The purpose of generator and coroutine are the same. Compare to normal functions that have to run code from start to end, these functions can pause in any place.

  1. Iterator and Generator I suggest we don't need to spend more time on improving the usage of async-operations in nginx module, now it is subrequest since there are a few different ways. (callback, promise, async/await, generator). It's better to implement that util these syntaxes are finished. Now, promise way looks OK.

What about starting to implement iterator and generator? If nobody is on it, I plan to do it. Welcome to your suggestions.

  1. parser refactoring BTW, I'm wondering it's ready to have a look at this patch? @lexborisov
drsm commented 4 years ago

@hongzhidao

Hi!

What about starting to implement iterator and generator? If nobody is on it, I plan to do it. Welcome to your suggestions.

Take a look at this example. I think, the main problem with iterator protocol will be a large number of temporaries generated. It is unusable without GC.

xeioex commented 4 years ago

Among Web APIs, the most interesting and promising is Web Crypto API.

hongzhidao commented 4 years ago

Among Web APIs, the most interesting and promising is Web Crypto API.

  1. It seems it's better to put these features into another place like njs-lib.c and njs-lib.h.
  2. Do you know what library is good to use for implementing Crypto? (OpenSSL?)
xeioex commented 4 years ago

@hongzhidao

Do you know what library is good to use for implementing Crypto? (OpenSSL?)

yes, ideally we want to use the same OpenSSL-like library which nginx is using. The trick is to get the right library (including the the libs that were provided as --with-openssl=...)

As on now njs configure does not see this information when it is built as a part of nginx (https://github.com/nginx/njs/blob/master/nginx/config.make).

hongzhidao commented 4 years ago

@xeioex

Just take a look at the implementation of Lua. lcrypto BTW, I'm thinking that some features like Web APIs URL, Request, etc even repl can be implemented with js since it needs more time by c way. url

xeioex commented 4 years ago

@drsm

I am thinking about the way to split-apart byte strings and ordinary strings (http://nginx.org/en/docs/njs/reference.html#string), because it is became increasingly complicated to handle them as a valid variants of a String. The similar issue with str vs bytes in python (https://stackoverflow.com/questions/18034272/python-str-vs-unicode-types).

Byte-string - a sequence of bytes. Ordinary strings - a unicode text string, internally represented as a valid UTF-8 string.

Historically byte string were introduced as as thin (non-copy) wrappers around nginx native objects (for example r.requestBody, r.variables.hostname) and we want this property to stay. We have a second requirement, all internally created strings are always a valid UTF-8 strings. Currently the only way to inject byte-strings is only through nginx modules.

The problems arise when we work with byte-strings internally (concating ordinary and byte strings, joining array which contains byte strings), when byte-string cannot be represented as a valid UTF-8 string (for example a byte '0x9d' is not a valid UTF-8).

There is also a backward compatibility issue, we should not change the current API too much.

For example:

  1. r.requestBody - what type should it be? currently it is a byte-string (consider JSON.stringify(r.requestBody) when r.requestBody is invalid UTF-8 string).
  2. r.variables - are also byte strings, because there are binary variables ($binary_remote_addr)
  3. s.on('upload', function (data, flags) {}) - data is also a byte string. ...

Ideally we want to get rid of byte-string altogether (replacing them with TypedArray). But there are API issues and performance issues. 1) TypedArray API is too different from String API, so a lot of existing code have to changes.

I am thinking about some kind of Proxy object (similar to Blob).

 NjsBlob 
   NjsBlob.prototype.arrayBuffer() 
   NjsBlob.prototype.text() -> convert to UTF8 string()
   NjsBlob.prototype.toString() -> text() // implicit conversion to String, but exception when it is not possible

 JSON.stringify(r.requestBody) // works out of the box

  r.requestBody.split('&') // requires changes, but seems manageable ->  r.requestBody.text().split('&')

NjsBlob != Blob - because the latter returns Promise(), seems too much overhead for simple things

Possibly adding later StringView.

What is your take on it?

drsm commented 4 years ago

@xeioex

I like a NjsBlob solution. Some thoughts:

  1. typeof r.requestBody == 'object'. I think it is better to freeze that object, as well as it's prototype, so the things can be optimized internally.
  2. r.requestBody + r.requestBody will be a UTF-8 string, so it's a breaking change. I think we need NjsBlob.from() and NjsBlob.concat() like nodejs Buffer does.
xeioex commented 4 years ago

@drsm

r.requestBody + r.requestBody will be a UTF-8 string, so it's a breaking change.

Yes, maybe implicit toString() is not a good idea.

> var a = new Uint8Array([0x41, 0x42]); var b = new Blob([a.buffer]); await b.text()
"AB"
> b.toString()
"[object Blob]"

I think we need NjsBlob.from() and NjsBlob.concat() like nodejs Buffer does.

Buffer is not a standard, ES6 version looks like:

const a = new Int8Array( [ 1, 2, 3 ] )
const b = new Int8Array( [ 4, 5, 6 ] )
const c = Int8Array.from([...a, ...b])
drsm commented 4 years ago

@xeioex

r.requestBody + r.requestBody will be a UTF-8 string, so it's a breaking change.

Yes, maybe implicit toString() is not a good idea.

Looks so, the problem that it will partitialy work.

Buffer is not a standard,

Yes, but it's well-known and it is a Uint8Array subclass in modern versions. And if we're dealing with node modules, browserify will bring its own implementation anyway.

drsm commented 4 years ago

@xeioex

 NjsBlob 
   NjsBlob.prototype.arrayBuffer() 
   NjsBlob.prototype.text() -> convert to UTF8 string()

Maybe is better to use properties instead?

NjsBlob 
    NjsBlob.prototype.size 
    NjsBlob.prototype.arrayBuffer 
    NjsBlob.prototype.text -> create/return a `string` representation
xeioex commented 4 years ago

@drsm

Maybe is better to use properties instead?

Yes, am considering it.

xeioex commented 4 years ago

@drsm

Meanwhile I am studying the way to port existing njs code automatically, it seems it is pretty straightforward in JS world.

For example, to robustly rename in the code String.bytesFrom() -> String.from():

cat example.js
function jwt(data) {
    return data.split('.').map(v=>String.
                           bytesFrom(v, 'base64url'));
}
$ jscodeshift -p -d -t njs-0.5.0-api-transform.js example.js 
Processing 1 files... 
Spawning 1 workers...
Running in dry mode, no files will be written! 
Sending 1 files to free worker...

The njs API transformer will do the following replacements:
Core:
        String.bytesFrom() -> String.from()

function jwt(data) {
    return data.split('.').map(v=>String.
                           from(v, 'base64url'));
}

All done. 
Results: 
0 errors
0 unmodified
0 skipped
1 ok
Time elapsed: 0.456seconds 
$ cat njs-0.5.0-api-transform.js
function transformer(file, api, options) {
  const j = api.jscodeshift;
  const source = j(file.source);

  console.log(`
The njs API transformer will do the following replacements:
Core:
    String.bytesFrom() -> String.from()

`);

  /*
   * String.bytesFrom() -> String.from();
   */
  source
    .find(j.CallExpression, {
      callee: {
        type: 'MemberExpression',
        object: {
          name: 'String'
        },
        property: {
          name: 'bytesFrom'
        }
      }
    })
    .replaceWith(path => { path.value.callee.property.name = 'from'; return path.value;});

  return source.toSource();
}

module.exports = transformer;
hongzhidao commented 4 years ago

Hi @xeioex @drsm

I'm on the support of the native HTTP client.

  1. Usage
    
    # http.js

function test(r) { r.fetch("http://127.1:8080", { method: "POST", headers: { "X-test": "xxxxx", "Y-test": "yyyyy" } }).then((reply) => { r.return(200, JSON.stringify(reply, null, 4)) }).catch(err => { r.return(200, err) }) } // reply: object, err: string

Result

curl http://127.1/test { "status": "200 OK", "headers": { "Server": "nginx/1.19.3", "Date": "Tue, 15 Sep 2020 09:00:20 GMT", "Content-Type": "text/plain", "Content-Length": "7", "Connection": "close" }, "body": "8080 ok"

My thoughts are:
1. make its usage close to the Web API.
With request and response, most of the formats are the same.

init is an object that includes some props and one headers which type is an object (string-string)

request(URL, init) response(body, init)

But it's overhead to implement `Response, Request, Headers, Body`, I prefer to simplify them as `object` and `string`.

2. APIs
`r.fetch` corresponds to the `request` mentioned above. Better naming are welcome.
`r.response` is a handy function with `r`. But I didn't implement it yet. Such as.
r.response("hello world", {
    status: 200,
    headers: {
        "X": "xx",
        "Y": "yy"
    }
})

3. Refactor
The file `njs_http_js_module.c` is so big that I prefer to separate some individual functions.
 ngx_module_name=ngx_http_js_module

ngx_module_incs="$ngx_addon_dir/../src \
                 $ngx_addon_dir/../build \
                 $ngx_addon_dir"

ngx_module_deps="$ngx_addon_dir/../build/libnjs.a \
                 $ngx_addon_dir/ngx_http_js.h"

ngx_module_srcs="$ngx_addon_dir/ngx_http_js.c \
                 $ngx_addon_dir/ngx_http_js_fetch.c \
                 $ngx_addon_dir/ngx_http_js_module.c"

ngx_module_libs="PCRE $ngx_addon_dir/../build/libnjs.a -lm"

The `ngx_http_js.h/c` is common.

4. Patch
This is a draft for the demo. BTW, most of the code borrows from ngx_event_openssl_stapling.c.
https://gist.github.com/hongzhidao/d534f1536494ea87973cb49218bf1e9f
I think `Keepalive`, `Connection-pool`, `Content-length`, `Chunked` should be supported.

5. Purpose
Though the `subrequest` can do the same thing, it's not flexible.
Communicating with 3rd API would make NJS more powerful.
I thought to support pure `socket` operations like `cosocket` in Lua, It's really useful.
But I think HTTP is good enough for most cases. Do it first.

Welcome to your suggestions.
hongzhidao commented 4 years ago

https://gist.github.com/hongzhidao/d534f1536494ea87973cb49218bf1e9f Updated.

  1. Removed ngx_http_js.c It seems njs_http_js.c is unnecessary for now.
  2. Implementation of creating request. Take advantage of chb, it's so useful.

BTW, take a look at njs_chb_test(...).

ret = njs_chb_join(&chain, &string);
/* string.start should be freed together with njs_chb_destroy() both in the case of success and fail. */

As I understand, chain is a temporary thing. It has an individual pool. Such as.

chain = njs_chb_create();
njs_chb_destroy(chain);
// njs_mp_free(string.start); no need to do this.
xeioex commented 4 years ago

@hongzhidao

Looks promising. Will take a look once I am with the Buffer object (most probably tomorrow).

hongzhidao commented 4 years ago

As I understand, chain is a temporary thing. It has an individual pool. Such as.

Sorry for my carelessness. This may copy the result one more time. It's good to use the same mem_pool.

In the case of only one node, what about processing without allocating a new memory?

diff -r eedb665fda25 src/njs_chb.c
--- a/src/njs_chb.c     Tue Sep 15 20:34:03 2020 -0400
+++ b/src/njs_chb.c     Wed Sep 16 23:08:39 2020 -0400
@@ -200,6 +200,12 @@
         return NJS_OK;
     }

+    if (n->next == NULL) {
+        str->length = njs_chb_node_size(n);
+        str->start = n->start;
+        return NJS_OK;
+    }
+
     size = (uint64_t) njs_chb_size(chain);
     if (njs_slow_path(size >= UINT32_MAX)) {
         return NJS_ERROR;

It's common that the client can receive a response body once, including chunked.

hongzhidao commented 4 years ago

@drsm @xeioex

var promise = new Promise(function(resolve, reject) {
    resolve()
})

promise.then((v) => {
    a()
})

In node.

UnhandledPromiseRejectionWarning: ReferenceError: a is not defined

In quickjs and njs, it works well. (It shows that njs_vm_call always returns OK mentioned bellow.)

Which way is correct?

I'm wondering if it always returns NJS_OK if the vm event is promise.

static void
ngx_http_js_handle_event(ngx_http_request_t *r, njs_vm_event_t vm_event,
    njs_value_t *args, njs_uint_t nargs)
{
    njs_int_t           rc;
    njs_str_t           exception;
    ngx_http_js_ctx_t  *ctx;

    ctx = ngx_http_get_module_ctx(r, ngx_http_js_module);

    njs_vm_post_event(ctx->vm, vm_event, args, nargs);

    rc = njs_vm_run(ctx->VM);

    if (rc == NJS_ERROR) {
        njs_vm_retval_string(ctx->vm, &exception);

        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
                      "js exception: %*s", exception.length, exception.start);

        ngx_http_finalize_request(r, NGX_ERROR);
    }

    if (rc == NJS_OK) {
        ngx_http_post_request(r, NULL);
    }
}

Since the ngx.fetch is related to ngx_http_js_handle_event. I'm finding a way of simplifying ngx_http_js_handle_event.

hongzhidao commented 4 years ago

Hi. Welcome to test ngx.fetch.

function test(r) {
    ngx.fetch("http://127.0.0.1:8080", {
        method: "POST",
        headers: {
            "X-test": "xxxxx",
            "Y-test": "yyyyy"
        }
    }).then((reply) => {
        r.return(200, JSON.stringify(reply))
    })
}

Patch. https://gist.github.com/hongzhidao/d534f1536494ea87973cb49218bf1e9f

Now, It's only used in HTTP module since the stream module does not support async mechanism (guess). And suggestions on how to use are welcome :)

drsm commented 4 years ago

@hongzhidao

Hi!

var promise = new Promise(function(resolve, reject) {
    resolve()
})

promise.then((v) => {
    a()
})

In node.

UnhandledPromiseRejectionWarning: ReferenceError: a is not defined

In quickjs and njs, it works well. (It shows that njs_vm_call always returns OK mentioned bellow.)

Which way is correct?

UnhandledPromiseRejectionWarning indicates an error actually. Starting from the nodejs 15 it will fail, see https://github.com/nodejs/node/pull/33021.

But all of them are correct, 'cause:

The default implementation of HostPromiseRejectionTracker is to unconditionally return an empty normal completion.

https://javascript.info/promise-error-handling

hongzhidao commented 4 years ago

@drsm For ngx.fetch, how do you want to use if an error happened? such as connected failed, timed out.

    ngx.fetch("http://127.0.0.1:8080", {
        method: "POST",
        headers: {
            "X-test": "xxxxx",
            "Y-test": "yyyyy"
        }
    }) // how to catch error? use HTTP status 500 with error message body?
drsm commented 4 years ago

@hongzhidao

looks like browser version doesn't throw anything, evenTypeErrors like wrong arguments. In case of error the result will be rejected promise with an exception.

    ngx.fetch("http://127.0.0.1:8080", {
        method: "POST",
        headers: {
            "X-test": "xxxxx",
            "Y-test": "yyyyy"
        }
    })
    .then((rs) => {
        // handle response
        if (rs.status >= 400) {
            var e =  new Error(`fetch failed: ${rs.status}`);
            e.response = rs;
            throw e;
        }
        // ...
    })
    .catch((e) => {
         // handle error
        if (e.response) {
             // server reports error
        } else {
             // timeout, DNS, etc...
        }
    })
hongzhidao commented 4 years ago
        // handle response
        if (rs.status >= 400) {
            var e =  new Error(`fetch failed: ${rs.status}`);
            e.response = rs;   // typo?
            throw e;
        }

Is this?

e.response = rs.body
drsm commented 4 years ago

Is this?

e.response = rs.body

There e.response is just a flag to indicate that we've got a response from server. It may be even e.response = true. If there is a need to distinguish 5xx from 4xx on error control path, we have to pass a full response, or only a status. So, it depends.

hongzhidao commented 4 years ago

@drsm Do you think it's worth to introduce r.repsponse in njs? https://developer.mozilla.org/en-US/docs/Web/API/Response/Response For example.

r.response("hello", {
      status: 200,
      headers: {
          X: "x",
          Y: "y"
      }
})
drsm commented 4 years ago

@hongzhidao

Yes, I think it would be nice to have some sugar like:

r.response({ some: thing })
// ==
r.response(JSON.stringify({ some: thing }), { status: 200, headers: {'content-type': 'application/json', ...})
// ;
r.response(Buffer.from('some bytes'), ...)
xeioex commented 3 years ago

@drsm, @hongzhidao

Hi guys, welcome to test Fetch API (ngx.fetch()).

What is supported: http and stream modules, chunked encoding. What is left to do: support async DNS resolve in urls (only ip as of now).

Added ngx.fetch().

This is an initial implementation of Fetch API.

The following properties and methods of Response object are implemented:
body, bodyUsed, headers, ok, status, type, url.

The following properties and methods of Header object are implemented:
get(), has().

The following properties and methods of Body object are implemented:
arrayBuffer(), json(), text().

Notable limitations: only http:// scheme is supported, redirects
are not handled.

In collaboration with 洪志道 (Hong Zhi Dao).

njs patch: https://gist.github.com/xeioex/f185ba904ee216e8490e20cf11c18e26 tests: https://gist.github.com/c224451823614d8701fb28d6a3290a8f nginx (only for stream module is needed): https://gist.github.com/3c377e927e01472f372e2c13ae2d4a8e

hongzhidao commented 3 years ago

@xeioex

njs_fetch:

  1. Added headers type check.
  2. Style. https://gist.github.com/hongzhidao/e0ee20282521d1eda56ad166d8c3590b

tests:

  1. Is this format a typo? Notice the char \ before $.
    ngx.fetch(`http://127.0.0.1:8080/\${loc}`, {headers: {Host: 'AAA'}})

    https://gist.github.com/xeioex/c224451823614d8701fb28d6a3290a8f#file-fetch_tests-patch-L119 If yes, there are some other places with the same problem.

  2. Unused location fetch. https://gist.github.com/xeioex/c224451823614d8701fb28d6a3290a8f#file-fetch_tests-patch-L119 The same to the new API. njs_vm_value_array_buffer_set https://gist.github.com/xeioex/f185ba904ee216e8490e20cf11c18e26#file-fetch-patch-L16

BTW, will do more test after the New Year vacation :)

xeioex commented 3 years ago

@hongzhidao

https://gist.github.com/hongzhidao/e0ee20282521d1eda56ad166d8c3590b

Applied, thanks.

Is this format a typo? Notice the char \ before $.

this is escaping for $ in perl string literals.

  1. Unused location fetch. The same to the new API. njs_vm_value_array_buffer_set

Good catches, thanks!

drsm commented 3 years ago

@xeioex

    ngx.fetch('http://104.28.2.142/r/hidden', { headers: { 'Host': 'requestbin.net' }})
    .then((res) => typeof res.headers.get('XXX'))
    .then((res) => r.return(200, res))
    .catch((e) => r.return(500, e.message)); // failed to convert text
  1. looks redundant and hides real cause.
  2. this should be:
    ret = ngx_response_js_ext_header_get(vm, njs_argument(args, 0),
                                         &name, njs_vm_retval(vm));
    if (ret == NJS_ERROR) {
        return NJS_ERROR;
    }
    return NJS_OK;

    Happy New Year!

hongzhidao commented 3 years ago

@xeioex Others look good.

BTW, I noticed that we need to care about the exception and throw it obviously, then return error status, such as.

njs_vm_error(vm, "internal error");
return NJS_ERROR;

If we can unify them into njs_value. It would be clearer, like the following:

njs_value_t *functions(vm, ...)
{
    return njs_throw_internal_error(vm, "internal error"); // njs_value_exception or njs_value_invalid
}

This's inspired by quickjs, most of the functions return with JSValue and passed JSValue parameters.

drsm commented 3 years ago

@xeioex @hongzhidao

Some issues I've found:

1.

The following properties and methods of Body object are implemented: arrayBuffer(), json(), text().

the Response object is a Body object, so arrayBuffer(), json(), text() should go uplevel.

  1. Headers problem:

    Unlike a header list, a Headers object cannot represent more than one Set-Cookie header. In a way this is problematic as unlike all other headers Set-Cookie headers cannot be combined, but since Set-Cookie headers are not exposed to client-side JavaScript this is deemed an acceptable compromise.

How about providing getAll and disable merging in get ?

xeioex commented 3 years ago

@drsm

How about providing getAll and disable merging in get ?

Not sure about disabling merging in get method. But, yes, getAll can be added.

xeioex commented 3 years ago

@hongzhidao

BTW, I noticed that we need to care about the exception and throw it obviously, then return error status, such as.

yes, because in JS you can throw anything, like throw 1;. So the value itself do not have enough information to distinguish thrown errors vs ordinary values.

drsm commented 3 years ago

@xeioex

Not sure about disabling merging in get method. But, yes, getAll can be added.

After some investigation I notice that any non-mergeable header except set-cookie is considered ill-formed :).

So, only getAll('set-cookie'): string[] is needed.

xeioex commented 3 years ago

@drsm, @hongzhidao, @jirutka

ngx.fetch() with DNS support.

 Added ngx.fetch().

This is an initial implementation of Fetch API.

The following init options are supported:
body, headers, max_response_body_size (nginx specific), method.

The following properties and methods of Response object are implemented:
arrayBuffer(), bodyUsed, json(), headers, ok, status, text(), type, url.

The following properties and methods of Header object are implemented:
get(), getAll(), has().

Notable limitations: only http:// scheme is supported, redirects
are not handled.

In collaboration with 洪志道 (Hong Zhi Dao).

njs patch: https://gist.github.com/98c4a9e8ae85c1a5a06669d7430de048 nginx-tests patch: https://gist.github.com/6a962569504c9a1f2701954aee479aa1 nginx (only for stream module is needed) patch: https://gist.github.com/3c377e927e01472f372e2c13ae2d4a8e

drsm commented 3 years ago

@xeioex

I found a problem, a duck disappear suddenly :)

$ curl localhost:10000/test
h[x-duck] = 🦆
response:
H[Connection] = close
H[x-duck] = 🦆
H[Host] = 127.0.0.1

$ curl localhost:10000/test
h[x-duck] = 🦆
response:
H[Connection] = close
H[x-duck] = 🦆
H[Host] = 127.0.0.1

$ curl localhost:10000/test
h[x-duck] = null
response:
H[Connection] = close
H[x-duck] = 🦆
H[Host] = 127.0.0.1
function dump(r) {
    var out = '';

    for (var k in r.headersIn) {
        out += `H[${k}] = ${r.headersIn[k]}\n`;
    }
    r.headersOut['x-duck'] = '🦆';
    r.return(200, out);
}

function test(r) {
    var out = '';

    ngx.fetch('http://127.0.0.1:10000/dump', { headers: { 'x-duck' : '🦆' } })
    .then((res) => {
        out += `h[x-duck] = ${res.headers.get('x-duck')}\n`;
        return res.text();
    })
    .then((res) => r.return(200, `${out}response:\n${res}\n`))
    .catch((e) => r.return(500, e.message));
}
xeioex commented 3 years ago

@drsm

Thanks:). The issue was trivial, I forgot to set a boolean value in a header struct. Failed to notice it because I prefer to run the nginx binary with address sanitizer enabled, which with high probability initialises a byte from dynamic memory with non-zero value.

--- a/nginx/ngx_js_fetch.c
+++ b/nginx/ngx_js_fetch.c
@@ -1083,6 +1083,7 @@ ngx_js_http_process_headers(ngx_js_http_
                 return NGX_ERROR;
             }

+            h->hash = 1;
             h->key.data = hp->header_name_start;
             h->key.len = hp->header_name_end - hp->header_name_start;
xeioex commented 3 years ago

@hongzhidao , @drsm

Committed, thank you very much for your contribution.

pyotruk commented 2 years ago

Hi all,

I couldn't get from the discussion whether you're actually planning to add support of the Web APIs? This would simplify our code a lot.

Currently, we are building an automatic convertor of Cloudflare JS workers to NJS workers. Cloudflare workers support Web API 100%, so we ended up writing polyfills for Request, Response, Headers and two adaptors.

Our index.ts looks like this:

import "core-js/stable/array/from";
import "core-js/stable/string/replace-all";
import URL from 'core-js-pure/stable/url';
import Request from "./request";
import Response from "./response";
import Headers from "./headers";
import convertNjsRequestToCloudflare from "./request-adaptor";
import {buildAndReturnNjsResponse, convertNjsResponseToCloudflare} from "./response-adaptor";

Please note, that some of the polyfills are already present in core-js library. So we basically only had to add Request, Response, Headers and the adaptors.

cc @ViieeS @ankit-globaldots

xeioex commented 2 years ago

Hi @vldmri,

We are planning to extend the WebAPI, but no specific plans yet. The plan includes:

hongzhidao commented 2 years ago

Hi @xeioex

Request, Response, Headers constructors

Is there any plan updated on the support of WebAPI?

Thanks.

xeioex commented 2 years ago

Hi @hongzhidao,

Yes, I am planning to add Request object support in the near-to-middle term future.

xeioex commented 2 years ago

Hi @hongzhidao,

BTW, can you please elaborate why you need a Request and Response object? A good usecase where Request is necessary or makes code much more readable will be enough.

hongzhidao commented 2 years ago

Hi @xeioex ,

We are supporting njs in Unit. we aren't using it in the same way as nginx. For example, users use template literal in options:

{
     "share": "`somepath${r.host + r.uri}`"
}

It's read-only. This is almost finished.

Then we plan to support read-write actions like r.return(status, text), but it hasn't been started. Internally, we are both considering the way of nginx and a new way like Web APIs. For example:

{
      "action": {
             "script": "some-module/foo"
      }
}
/* JS module */

var m = {}

m.foo = function(req) {
    return new Response("hello", {
        status: 200,
        headers: {
          'content-type': 'application/json;charset=UTF-8'
        }
    })
}

export default m

can you please elaborate why you need a Request and Response object?

It's not clear now, We'll make the decision clear internally first. Maybe we can do some polyfills to check how it will be in Unit. But we won't start Web APIs until at least next month. We are still on the template-literal stuff. And if we decide the way of Web APIs, we will not need to support the way like nginx r.send(), etc. We support only one usage to users, anyway.

Btw, you have a lot of experience in njs usage with nginx, your advice on how to use it in Unit will very useful. Welcome to suggest. Thanks!

tippexs commented 2 years ago

@vldmri Do you have any plans to open source the polyfills? We are thinking about an similiar implemenation for NGINX Unit while NJS is not supporting it natively. Just wanted to check in with you before starting any of this work. Thanks for your feedback.

jirutka commented 2 years ago

Hi @hongzhidao, you may be interested in nginx-testing and njs-typescript-starter.

ViieeS commented 2 years ago

@tippexs you can use these polyfills. However, you need to compile it properly, we based our pipeline on @jirutka njs-typescript-starter. Also you may need request/response adapters

https://www.npmjs.com/package/headers-polyfill https://gist.github.com/ViieeS/139057f70bf98b66295b73bc3b30c391

tippexs commented 2 years ago

I am aware of the typescript starter and modified a couple of things in my typescript setup! We should meet at our Nginx community slack and chat about it. Would look forward to it.

Thanks for the links and I will have a look on it! Have a great weekend! Timo