Add TypeScript types for modules #342

jirutka · 2020-10-09T00:35:41Z

Questions:

What is type Buffer – ArrayBuffer or some custom type?
Where should be used NjsStringLike and where string?

Patch: https://patch-diff.githubusercontent.com/raw/nginx/njs/pull/342.patch

jirutka · 2020-10-14T19:45:15Z

@xeioex, could you please give me some feedback and answer the questions above?

xeioex · 2020-10-14T19:54:39Z

@jirutka

Sorry for delay, I plan to return to your merge request later.

What is type Buffer – ArrayBuffer or some custom type?

No, it replicates API for node.js Buffer

NjsStringLike

Basically it is a pseudotype for TS, which says that some function accepts both string and NjsByteString.

In the future, we plan to get rid of NjsByteString (as a non-standard type) in favour of Buffer.

xeioex · 2020-10-14T19:56:36Z

@jirutka

Regarding the modules API, we are trying to follow the original API for them, can't we just copy the existing API description for them?

jirutka · 2020-10-14T20:11:04Z

Thanks for the quick response!

No, it replicates API for node.js Buffer

Okay, I will update the type definitions.

Basically it is a pseudotype for TS, which says that some function accepts both string and NjsByteString.

I understand that. I just don’t know how to recognize what functions accept/return NjsByteString and what functions just plain string.

Regarding the modules API, we are trying to follow the original API for them, can't we just copy the existing API description for them?

You mean Node.js API? You don’t follow them fully, there are some minor differences (e.g. missing functions, some extra arguments not supported, limited set of encodings etc.). It’s more a subset of them. The TS types must to be accurate to be useful.

BTW, I have copied JSDoc comments from http://nginx.org/en/docs/njs/reference.html, not Node.js docs, both due to differences and to avoid potential legal issues (not sure if there are any, just to be sure).

jirutka · 2020-10-14T20:29:27Z

No, it replicates API for node.js Buffer

Node’s Buffer has quite a large API surface – does njs support all of its (non-deprecated) methods with all arguments? What encodings does it support?

xeioex · 2020-10-14T20:36:20Z

@jirutka

I just don’t know how to recognize what functions accept/return NjsByteString and what functions just plain string.

yes, that is the point of deprecation of NjsByteString type.

Currently, we basically have two types string and NjsByteString implemented in a single String.prototype.

The only difference is the way they treat characters.
NjsByteString - a character is always a single byte.
string - a character is a UTF8 codepoint (this is not the same as UTF16 from the spec, but the visible difference is pretty negligible, only rarely used surrogate pairs are affected).

String.prototype.toUTF8(), and String.prototype.fromUTF8() are used to convert in both directions.

xeioex · 2020-10-14T20:41:15Z

@jirutka

Node’s Buffer has quite a large API surface – does njs support all of its (non-deprecated) methods with all arguments? What encodings does it support?

We plan to release the documentation for it by the end of this week.
Most of the (non-deprecated) methods are supported.

With the exception of: buf.keys(), buf.entries(), buf.values(), buf.readBigInt64BE() (and other BigInt methods).

The available encodings are: utf8, hex, base64, base64url.

jirutka · 2020-10-14T20:50:21Z

BTW, I haven’t tested it yet, but it should be possible to generate TS types and HTML documentation from TSDoc comments included directly in the .c files. This would help keep implementation, types and documentation in-sync. Would you be interested in this approach?

xeioex · 2020-10-14T20:56:13Z

@jirutka

It would be great if you share a working POC (using a .c file and TSDoc).

xeioex · 2020-10-16T13:01:04Z

http://nginx.org/en/docs/njs/reference.html#buffer - Buffer docs.

jirutka · 2020-10-16T23:45:55Z

Okay, I’ve just added the Buffer type to this PR.

jirutka · 2020-10-17T00:00:24Z

If I understand njs_buffer.c correctly, Buffer.prototype.write returns number, but all write* methods for numbers return void. However, in Node’s Buffer, all write* methods return number. Is this intentional…?

lexborisov · 2020-10-17T06:57:45Z

Hi @jirutka

This is my mistake, I will fix it soon.
Thanks for the report!

lexborisov · 2020-10-17T07:34:14Z

@jirutka

Try this patch:

# HG changeset patch
# User Alexander Borisov <alexander.borisov@nginx.com>
# Date 1602919869 -10800
#      Sat Oct 17 10:31:09 2020 +0300
# Node ID b4429c95c89a1b54da3450aef85db96d0f650d23
# Parent  a82f123409b7c0c8b97417e11dffde523018ed30
Fixed returned value for Buffer.prototype.write* functions.

All Buffer.prototype.write* functions must return offset plus
the number of bytes written.

The issue was introduced in 27bb9caf186c.

diff -r a82f123409b7 -r b4429c95c89a src/njs_buffer.c
--- a/src/njs_buffer.c	Tue Oct 13 15:44:33 2020 +0300
+++ b/src/njs_buffer.c	Sat Oct 17 10:31:09 2020 +0300
@@ -1331,7 +1331,7 @@ njs_buffer_prototype_write_int(njs_vm_t 
         break;
     }
 
-    njs_set_undefined(&vm->retval);
+    njs_set_number(&vm->retval, index + size);
 
     return NJS_OK;
 }
diff -r a82f123409b7 -r b4429c95c89a src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c	Tue Oct 13 15:44:33 2020 +0300
+++ b/src/test/njs_unit_test.c	Sat Oct 17 10:31:09 2020 +0300
@@ -19191,7 +19191,11 @@ static njs_unit_test_t  njs_test[] =
               "        if (size > 1) { rmethod += endianness; wmethod += endianness; };"
               "        var v = 0x7abbccddeeff & (size * 8 - 1);"
               ""
-              "        buf[wgmethod](v, offset, size);"
+              "        var ret = buf[wgmethod](v, offset, size);"
+              "        if(ret !== offset + size) {"
+              "            throw Error(`${wgmethod} returned ${ret}, need ${offset + size}`);"
+              "        }"
+              ""
               "        var gv = buf[rgmethod](offset, size);"
               ""
               "        buf.fill(0);"

src/ts/fs.d.ts

jirutka · 2020-10-18T17:27:21Z

I’ve polished it a bit and I think that it’s ready.

src/ts/njs_core.d.ts

drsm · 2020-10-19T19:20:03Z

@xeioex
@jirutka

Hi!

Looks like all the paths in fs module should be of type export type PathLike = string | Buffer instead of NjsStringLike

jirutka · 2020-10-19T19:22:52Z

I’ve fixed all the mentioned issues in the crypto and querystring modules.

It seems that NjsStringLike is used basically as in input type only (function parameters), not as the return type, right? If so, the following in the fs module should be changed too, right?

- readFileSync(path: NjsStringLike, options: { encoding?: FileEncoding; flag?: OpenMode; } | FileEncoding): NjsStringLike;
+ readFileSync(path: NjsStringLike, options: { encoding?: FileEncoding; flag?: OpenMode; } | FileEncoding): string;

- readdirSync(path: NjsStringLike, options?: { encoding?: "utf8"; withFileTypes?: false; } | "utf8"): NjsStringLike[];
+ readdirSync(path: NjsStringLike, options?: { encoding?: "utf8"; withFileTypes?: false; } | "utf8"): string[];

- realpathSync(path: NjsStringLike, options?: { encoding?: "utf8" } | "utf8"): NjsStringLike;
+ realpathSync(path: NjsStringLike, options?: { encoding?: "utf8" } | "utf8"): string;

- readFile(path: NjsStringLike, options: { encoding?: FileEncoding; flag?: OpenMode; } | FileEncoding): Promise<NjsStringLike>;
+ readFile(path: NjsStringLike, options: { encoding?: FileEncoding; flag?: OpenMode; } | FileEncoding): Promise<string>;

- readdir(path: NjsStringLike, options?: { encoding?: "utf8"; withFileTypes?: false; } | "utf8"): Promise<NjsStringLike[]>;
+ readdir(path: NjsStringLike, options?: { encoding?: "utf8"; withFileTypes?: false; } | "utf8"): Promise<string[]>;

- realpath(path: NjsStringLike, options?: { encoding?: "utf8" } | "utf8"): Promise<NjsStringLike>;
+ realpath(path: NjsStringLike, options?: { encoding?: "utf8" } | "utf8"): Promise<string>;

I’ve also changed Buffer module to extend Uint8Array (as in Node.js) and accept Uint8Array instead of Buffer (its subtype) in relevant functions, after I’ve noticed in njs shell that it really behaves in the same way as in Node regarding Uint8Array.

xeioex · 2020-10-19T19:31:59Z

It seems that NjsStringLike is used basically as in input type only (function parameters), not as the return type, right?

@jirutka yes, since 0.4.4 "fs" method ensures that ordinary string are returned (as a part of the plan to eliminate NjsByteString).

But, still NjsByteString can be created with deprecated method String.bytesFrom() (will be removed in the future).

Created according to http://nginx.org/en/docs/njs/reference.html. Signed-off-by: Jakub Jirutka <jakub@jirutka.cz>

nginx#342 (comment)

jirutka · 2020-10-19T19:49:24Z

Okay, updated: all the path parameters accept PathLike (PathLike = string | Buffer), functions don’t return NjsStringLike, but string, NjsStringLike is used only for the input data parameters.

I’ve also tagged String.bytesFrom() as @deprecated.

src/ts/fs.d.ts

Created according to http://nginx.org/en/docs/njs/reference.html. Signed-off-by: Jakub Jirutka <jakub@jirutka.cz>

nginx#342 (comment)

xeioex · 2020-10-20T15:39:25Z

nginx/ts/ngx_stream_js_module.d.ts

@@ -66,7 +66,7 @@ interface NginxStreamVariables {
    readonly 'time_iso8601'?: NjsByteString;
    readonly 'time_local'?: NjsByteString;

-    [prop: string]: NjsByteString;
+    [prop: string]: NjsByteString | undefined;


@jirutka Can you share the exact code snippet that causes the issue?

Sure, it’s part of #344:

git clone -b njs-types-pkg git@github.com:jirutka/njs.git cd njs/ts npm install npm test

The problem is that you’re running tsc with the defaults – backward compatible settings that are very lax and shouldn’t be used for any new projects.

Sorry for mixing this commit into this PR, I should have created a new PR for it.

I’m gonna move this commit into #344 and close this PR, ’cause it has been already merged, except this one.

xeioex · 2020-10-20T15:56:06Z

@jirutka BTW, is there a proper way to reference internal types like Hash? https://github.com/nginx/njs/blob/master/test/ts/test.ts#L87

Something like:

var h:Hash;

h = crypto.createHash('sha1');

jirutka · 2020-10-20T19:45:21Z

@jirutka BTW, is there a proper way to reference internal types like Hash? https://github.com/nginx/njs/blob/master/test/ts/test.ts#L87

Something like:
var h:Hash;

h = crypto.createHash('sha1');

Well, Hash is exported, so it’s not an internal type, you can just import it:

import crypto, { Hash } from 'crypto'

const h: Hash = crypto.createHash('sha1')

TypeScript will automatically remove named imports of types from the generated ES2015 code.

jirutka · 2020-10-22T17:40:36Z

Merged in 993e7a8, 20c02d0, ef46a05, e246283, fb7ba09.

jirutka changed the title ~~WIP: Add TypeScript types for modules~~ Add TypeScript types for modules Oct 11, 2020

xeioex added the docs label Oct 14, 2020

jirutka force-pushed the types branch from e387a42 to ae491aa Compare October 14, 2020 21:01

jirutka force-pushed the types branch from ae491aa to e5fda9b Compare October 16, 2020 23:44

jirutka force-pushed the types branch 3 times, most recently from cc5e787 to 2496cce Compare October 17, 2020 00:00

jirutka force-pushed the types branch 2 times, most recently from 2443e3c to fd5c0c7 Compare October 18, 2020 17:25

jirutka commented Oct 18, 2020

View reviewed changes

src/ts/fs.d.ts Outdated Show resolved Hide resolved

jirutka commented Oct 18, 2020

View reviewed changes

src/ts/fs.d.ts Outdated Show resolved Hide resolved

jirutka force-pushed the types branch 3 times, most recently from f74ca06 to 033ce0a Compare October 18, 2020 17:56

xeioex requested changes Oct 19, 2020

View reviewed changes

src/ts/njs_core.d.ts Outdated Show resolved Hide resolved

src/ts/njs_core.d.ts Outdated Show resolved Hide resolved

src/ts/njs_core.d.ts Show resolved Hide resolved

jirutka force-pushed the types branch 2 times, most recently from 372811f to 8f4fc6d Compare October 19, 2020 19:07

Types: Add type Buffer to njs_core

f8496ab

Created according to http://nginx.org/en/docs/njs/reference.html. Signed-off-by: Jakub Jirutka <jakub@jirutka.cz>

jirutka added a commit to jirutka/njs that referenced this pull request Oct 19, 2020

Types: Tag String.bytesFrom() in as @deprecated

a1bbb58

nginx#342 (comment)

jirutka force-pushed the types branch from 8f4fc6d to a1bbb58 Compare October 19, 2020 19:45

drsm reviewed Oct 19, 2020

View reviewed changes

src/ts/fs.d.ts Outdated Show resolved Hide resolved

jirutka added 4 commits October 19, 2020 22:53

Types: Add types for fs module

9711ae9

Created according to http://nginx.org/en/docs/njs/reference.html. Signed-off-by: Jakub Jirutka <jakub@jirutka.cz>

Types: Add types for querystring module

230ed5f

Created according to http://nginx.org/en/docs/njs/reference.html. Signed-off-by: Jakub Jirutka <jakub@jirutka.cz>

Types: Add types for crypto module

f90044b

Created according to http://nginx.org/en/docs/njs/reference.html. Signed-off-by: Jakub Jirutka <jakub@jirutka.cz>

Types: Tag String.bytesFrom() in as @deprecated

2b9b808

nginx#342 (comment)

jirutka force-pushed the types branch from a1bbb58 to 2b9b808 Compare October 19, 2020 20:54

xeioex reviewed Oct 20, 2020

View reviewed changes

jirutka force-pushed the types branch from 3c4a96c to 2b9b808 Compare October 22, 2020 17:38

jirutka closed this Oct 22, 2020

jirutka deleted the types branch October 22, 2020 17:40

jirutka mentioned this pull request Oct 22, 2020

Prepare TS types for publishing on npm #344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeScript types for modules #342

Add TypeScript types for modules #342

jirutka commented Oct 9, 2020 •

edited

Loading

jirutka commented Oct 14, 2020

xeioex commented Oct 14, 2020

xeioex commented Oct 14, 2020

jirutka commented Oct 14, 2020

jirutka commented Oct 14, 2020 •

edited

Loading

xeioex commented Oct 14, 2020 •

edited

Loading

xeioex commented Oct 14, 2020 •

edited

Loading

jirutka commented Oct 14, 2020

xeioex commented Oct 14, 2020

xeioex commented Oct 16, 2020

jirutka commented Oct 16, 2020

jirutka commented Oct 17, 2020

lexborisov commented Oct 17, 2020 •

edited

Loading

lexborisov commented Oct 17, 2020

jirutka commented Oct 18, 2020

drsm commented Oct 19, 2020

jirutka commented Oct 19, 2020

xeioex commented Oct 19, 2020 •

edited

Loading

jirutka commented Oct 19, 2020

xeioex Oct 20, 2020

jirutka Oct 20, 2020

jirutka Oct 20, 2020

jirutka Oct 22, 2020

xeioex commented Oct 20, 2020

jirutka commented Oct 20, 2020

jirutka commented Oct 22, 2020

Add TypeScript types for modules #342

Add TypeScript types for modules #342

Conversation

jirutka commented Oct 9, 2020 • edited Loading

jirutka commented Oct 14, 2020

xeioex commented Oct 14, 2020

xeioex commented Oct 14, 2020

jirutka commented Oct 14, 2020

jirutka commented Oct 14, 2020 • edited Loading

xeioex commented Oct 14, 2020 • edited Loading

xeioex commented Oct 14, 2020 • edited Loading

jirutka commented Oct 14, 2020

xeioex commented Oct 14, 2020

xeioex commented Oct 16, 2020

jirutka commented Oct 16, 2020

jirutka commented Oct 17, 2020

lexborisov commented Oct 17, 2020 • edited Loading

lexborisov commented Oct 17, 2020

jirutka commented Oct 18, 2020

drsm commented Oct 19, 2020

jirutka commented Oct 19, 2020

xeioex commented Oct 19, 2020 • edited Loading

jirutka commented Oct 19, 2020

xeioex Oct 20, 2020

Choose a reason for hiding this comment

jirutka Oct 20, 2020

Choose a reason for hiding this comment

jirutka Oct 20, 2020

Choose a reason for hiding this comment

jirutka Oct 22, 2020

Choose a reason for hiding this comment

xeioex commented Oct 20, 2020

jirutka commented Oct 20, 2020

jirutka commented Oct 22, 2020

jirutka commented Oct 9, 2020 •

edited

Loading

jirutka commented Oct 14, 2020 •

edited

Loading

xeioex commented Oct 14, 2020 •

edited

Loading

xeioex commented Oct 14, 2020 •

edited

Loading

lexborisov commented Oct 17, 2020 •

edited

Loading

xeioex commented Oct 19, 2020 •

edited

Loading