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

Support Web APIs. #299

Open
hongzhidao opened this issue Mar 27, 2020 · 50 comments
Open

Support Web APIs. #299

hongzhidao opened this issue Mar 27, 2020 · 50 comments

Comments

@hongzhidao
Copy link
Contributor

hongzhidao commented Mar 27, 2020

@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
Copy link
Contributor

xeioex commented Mar 27, 2020

@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
Copy link
Contributor Author

@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
Copy link
Contributor

drsm commented Apr 6, 2020

@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
Copy link
Contributor

xeioex commented Jun 5, 2020

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

@hongzhidao
Copy link
Contributor Author

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
Copy link
Contributor

xeioex commented Jun 8, 2020

@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
Copy link
Contributor Author

@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
Copy link
Contributor

xeioex commented Jul 15, 2020

@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
Copy link
Contributor

drsm commented Jul 16, 2020

@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
Copy link
Contributor

xeioex commented Jul 16, 2020

@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
Copy link
Contributor

drsm commented Jul 16, 2020

@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
Copy link
Contributor

drsm commented Jul 17, 2020

@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
Copy link
Contributor

xeioex commented Jul 17, 2020

@drsm

Maybe is better to use properties instead?

Yes, am considering it.

@xeioex
Copy link
Contributor

xeioex commented Jul 17, 2020

@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
Copy link
Contributor Author

hongzhidao commented Sep 15, 2020

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.

  1. 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"
        }
    })
  1. 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.

  1. 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.

  2. 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
Copy link
Contributor Author

hongzhidao commented Sep 16, 2020

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
Copy link
Contributor

xeioex commented Sep 16, 2020

@hongzhidao

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

@hongzhidao
Copy link
Contributor Author

hongzhidao commented Sep 17, 2020

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
Copy link
Contributor Author

hongzhidao commented Sep 29, 2020

@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
Copy link
Contributor Author

hongzhidao commented Sep 30, 2020

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
Copy link
Contributor

drsm commented Sep 30, 2020

@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 nodejs/node#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
Copy link
Contributor Author

hongzhidao commented Sep 30, 2020

@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
Copy link
Contributor

drsm commented Sep 30, 2020

@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
Copy link
Contributor Author

        // 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
Copy link
Contributor

drsm commented Sep 30, 2020

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
Copy link
Contributor Author

hongzhidao commented Oct 6, 2020

@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
Copy link
Contributor

drsm commented Oct 6, 2020

@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
Copy link
Contributor

xeioex commented Dec 29, 2020

@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
Copy link
Contributor Author

hongzhidao commented Dec 30, 2020

@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
Copy link
Contributor

xeioex commented Dec 30, 2020

@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
Copy link
Contributor

drsm commented Dec 31, 2020

@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
Copy link
Contributor Author

hongzhidao commented Jan 10, 2021

@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
Copy link
Contributor

drsm commented Jan 10, 2021

@xeioex
@hongzhidao

Some issues I've found:

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
Copy link
Contributor

xeioex commented Jan 10, 2021

@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
Copy link
Contributor

xeioex commented Jan 11, 2021

@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
Copy link
Contributor

drsm commented Jan 11, 2021

@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
Copy link
Contributor

xeioex commented Jan 13, 2021

@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
Copy link
Contributor

drsm commented Jan 14, 2021

@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
Copy link
Contributor

xeioex commented Jan 18, 2021

@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
Copy link
Contributor

xeioex commented Jan 21, 2021

@hongzhidao , @drsm

Committed, thank you very much for your contribution.

@pyotruk
Copy link

pyotruk commented Feb 14, 2022

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
Copy link
Contributor

xeioex commented Feb 14, 2022

Hi @vldmri,

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

  • Request, Response, Headers constructors
  • adding Request object support as argument in ngx.fetch()

@hongzhidao
Copy link
Contributor Author

Hi @xeioex

Request, Response, Headers constructors

Is there any plan updated on the support of WebAPI?

Thanks.

@xeioex
Copy link
Contributor

xeioex commented Aug 16, 2022

Hi @hongzhidao,

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

@xeioex
Copy link
Contributor

xeioex commented Aug 24, 2022

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
Copy link
Contributor Author

hongzhidao commented Aug 24, 2022

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
Copy link

tippexs commented Aug 26, 2022

@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
Copy link
Contributor

jirutka commented Aug 26, 2022

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

@ViieeS
Copy link

ViieeS commented Aug 26, 2022

@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
Copy link

tippexs commented Aug 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants