Njs: Patch: adapted TypeScript descriptions for recent changes.

[xeioex]

@jirutka @drsm Feedback is welcome.

Updated: https://gist.github.com/xeioex/47d46bf28b6a0f12847bd97d85bfa1a8

The last patch which adds Buffer variants of existing properties (r.vars, r.reqBody ..) is missing.

I need to add a variant of NginxVariables interface, the only difference that the value type of a returned variable is Buffer:

interface NginxVars {
    readonly 'ancient_browser'?: Buffer;
   ...

Is there a good way to avoid duplication of the full list here?

[jirutka]

diff --git a/ts/ngx_http_js_module.d.ts b/ts/ngx_http_js_module.d.ts
@@ -1,4 +1,5 @@
/// <reference path="index.d.ts" />
+/// <reference path="ngx_core.d.ts" />

diff --git a/ts/ngx_stream_js_module.d.ts b/ts/ngx_stream_js_module.d.ts
@@ -1,4 +1,5 @@
/// <reference path="index.d.ts" />
+/// <reference path="ngx_core.d.ts" />

These changes are unnecessary, index.d.ts references ngx_core.d.ts, so its already included. (https://github.com/nginx/njs/issues/358#issuecomment-733887651)

Is there a good way to avoid duplication of the full list here?

Yes, there is, I will look at it in a while (busy now).

[xeioex]

@jirutka

These changes are unnecessary, index.d.ts references ngx_core.d.ts, so its already included.

ngx object is not available in njs CLI.
njs_core.d.ts - includes built-in njs objects.
ngx_core.d.ts - common objects for nginx modules.

[drsm]

@xeioex

maybe rename NjsBufferLike to NjsStringOrBuffer ?
'cause it's either a string or a buffer of some kind (Buffer, ByteString, etc...).

[jirutka]

These changes are unnecessary, index.d.ts references ngx_core.d.ts, so its already included.

ngx object is not available in njs CLI.

Aha, it’s _ngx_core_, not _njs_core_! Mea culpa. Okay, but where is ngx_core.d.ts?

The last patch which adds Buffer variants of existing properties (r.vars, r.reqBody ..) is missing.

Here it is https://gist.github.com/jirutka/898aa0dd231a5af5019715082a0a4004.

[jirutka]

What’s the plan with requestBody vs. reqBody and responseBody vs. resBody? I assume that the new ones deprecate the old ones, right? Will you eventually (in 1.0.0) rename reqBody to requestBody and keep reqBody (and maybe keep reqBody as a deprecated alias for requestBody), for consistency in naming?

What about variables vs. vars? I understand the transition from NjsByteString to Buffer in request/response body, but I find it very impractical in variables. Most of them will always contain just ASCII or UTF8 strings and users will be basically forced to call .toString() over and over again.

[xeioex]

@jirutka

What about variables vs. vars? I understand the transition from NjsByteString to Buffer in request/response body

There are binary variables like binary_remote_addr, session_log_binary_id, etc. In the future, NjsByteString will be removed, and all strings will be forced to valid UTF-8, possibly corrupting some byte sequences, so we need a Buffer variant.

but I find it very impractical in variables

This is a valid concern. I think, we may leave r.variables along with r.vars.

[xeioex]

@jirutka

Aha, it’s ngx_core, not njs_core! Mea culpa. Okay, but where is ngx_core.d.ts?

Yes, I forgot to add ngx_core.d.ts to the commit, here is updated version
https://gist.github.com/xeioex/47d46bf28b6a0f12847bd97d85bfa1a8

[jirutka]

There are binary variables like binary_remote_addr, session_log_binary_id, etc.

What about using Buffer for these binary variables and plain string for others?

[drsm]

@jirutka

What about using Buffer for these binary variables and plain string for others?

Any user defined variable may be set or mapped to binary one in config.

BTW, maybe rawVariables ?

[jirutka]

BTW, maybe rawVariables?

This is definitely a better name! :+1:

[drsm]

@xeioex

Is there a good way to avoid duplication of the full list here?

interface VarDict<T> {
    readonly 'ancient_browser'?: T;
    readonly 'binary_remote_addr'?: T;
    readonly 'body_bytes_sent'?: T;
    readonly 'bytes_received'?: T;
    readonly 'bytes_sent'?: T;

    readonly [name: string]: T;
};

type TestVars = VarDict<string>;
type TestRawVars = VarDict<Buffer>;

var v: TestVars;
var rv: TestRawVars;
var s: string;
var b: Buffer;

s = v['body_bytes_sent'];
b = rv['binary_remote_addr'];

[jirutka]

@drsm, since ngx_http_js_modules.d.ts is not a module (there’s no export), every type declaration is “exported” as global. Thus VarDict would be available as a global type which is not good because it’s just an internal helper. The solution I proposed in https://github.com/nginx/njs/issues/358#issuecomment-733887651 doesn’t need any extra type.

[xeioex]

@drsm

BTW, maybe rawVariables ?

Sounds much better, agree. Committed.

[jirutka]

@xeioex, I’ve updated https://gist.github.com/jirutka/898aa0dd231a5af5019715082a0a4004 to incorporate rename of vars to rawVariables.

[xeioex]

@jirutka

Here is my version: https://gist.github.com/3707493dcbe9d027146addc185459069. (I also expanded description for s.on(), s.off() for the stream module).

Thanks for the original patch.

[jirutka]

Here is my version: https://gist.github.com/3707493dcbe9d027146addc185459069.

+     * @param event Event type to register. The the callback data value type
                                                 ^-- typo

+       callback: (data: Buffer,  flags: NginxStreamCallbackFlags) => void): void;
                                 ^-- two spaces

[drsm]

@xeioex
@jirutka

What’s the plan with requestBody vs. reqBody and responseBody vs. resBody? I assume that the new ones deprecate the old ones, right? Will you eventually (in 1.0.0) rename reqBody to requestBody and keep reqBody (and maybe keep reqBody as a deprecated alias for requestBody), for consistency in naming?

Some random thoughts around this:

• Alias responseBody = responseText; requestBody = requestText
• Add responseBuffer; requestBuffer instead of reqBody, resBody
• Add some sugar responseObject; requestObject
• Drop requestBody, responseBody

[xeioex]

@drsm

Very good proposal. Committed.

Updated patch for TS to review: https://gist.github.com/f7bfa6435ddb44871bbc0533712920f2.
@jirutka