Prepare TS types for publishing on npm #344

jirutka · 2020-10-20T00:08:55Z

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

xeioex · 2020-10-21T13:24:33Z

@jirutka
./configure && make ts_test

test/ts/test.ts:1:16 - error TS2307: Cannot find module 'fs'.

1 import fs from 'fs';
...
Found 19 errors.

jirutka · 2020-10-21T13:36:19Z

This PR is not ready yet.

jirutka · 2020-10-21T14:06:15Z

You can try it now. However, it’s still not ready for merge.

I wanted to ask, would it be acceptable to use npm install in ts_test to install the specified version of typescript from npm (and possibly other test dependencies in the future) and link njs-types from local directory?

xeioex · 2020-10-21T14:28:23Z

@jirutka

I wanted to ask, would it be acceptable to use npm install in ts_test to install the specified version of typescript from npm (and possibly other test dependencies in the future) and link njs-types from local directory?

yes, this is acceptable because it is for developers only, and should be called explicitly (this should not be a dependency of any common targets like njs, test etc).

njs-types from local directory?

from build directory, right? this should be the only modifiable directory.

jirutka · 2020-10-22T12:26:18Z

yes, this is acceptable because it is for developers only, and should be called explicitly (this should not be a dependency of any common targets like njs, test etc).

Okay, great!

from build directory, right? this should be the only modifiable directory.

Okay, I’ve modified the Makefile in this regards.

It’s almost ready. Please take a look if this approach makes sense to you.

ts/ngx_http_js_module.d.ts

jirutka · 2020-10-24T20:16:31Z

I’ve originally suggested using the version number in format <njs-version>-<types-rev> (e.g. 0.4.2-1) to maintain one-to-one relation between njs and njs-types versions and at the same time allow releasing new revisions of njs-types between njs releases. However, I eventually came to the conclusion that it was a bad idea.

npmjs enforces SemVer which doesn’t allow four-part version number nor provide post-release suffixes. The version suffix -<types-rev> actually denotes a pre-release version. I thought that it can be used in this case assuming that you will append this suffix to each released version, i.e. you will not release e.g. 0.4.2 (which has higer precedence than 0.4.2-1, 0.4.2-2…). The problem is that -1, -2, … has lower precedence even than -dev.2, .beta.3 etc. And it would be also a bit confusing for the users because ^0.4.2 doesn’t include 0.4.2-1.

For these reasons, I think that it will be better use a similar approach as DefinitelyTyped. I’ve described it in ts/README.md:

njs-types is typically being released together with njs. Their major and minor release numbers (the first two numbers) are always aligned, but the patch version (the third number) may differ. That's because njs-types may be updated between njs releases and in such case the patch version is incremented.

When ./ts is copied into ./build/ts, the version in package.json is substituted with version read from src/njs.h. This can be overriden using make variable NJS_TYPES_VER (e.g. ./configure && make ts_publish NJS_TYPES_VER=0.4.2).

xeioex · 2020-10-26T19:47:44Z

@jirutka

Here are my improvements over your work
https://gist.github.com/xeioex/ea94f68579a857e58143e24511b8f6db

I am still working on it (planning to merge and simplify some patches further)

But, I am have an issue right now with modules

$ make ts_test
...
test.ts:1:16 - error TS2307: Cannot find module 'fs' or its corresponding type declarations.

1 import fs from 'fs';
                 ~~~~

test.ts:2:16 - error TS2307: Cannot find module 'querystring' or its corresponding type declarations.

2 import qs from 'querystring';
                 ~~~~~~~~~~~~~

test.ts:3:20 - error TS2307: Cannot find module 'crypto' or its corresponding type declarations.

3 import crypto from 'crypto';
                     ~~~~~~~~


Found 3 errors.

jirutka · 2020-10-26T20:25:46Z

Well, that’s because you’ve excluded the following changes (- is this PR, + is yours):

--- a/ts/ngx_http_js_module.d.ts
+++ b/ts/ngx_http_js_module.d.ts
@@ -1,4 +1,4 @@
-/// <reference path="index.d.ts" />
+/// <reference path="njs_core.d.ts" />
--- a/ts/ngx_stream_js_module.d.ts
+++ b/ts/ngx_stream_js_module.d.ts
@@ -1,4 +1,4 @@
-/// <reference path="index.d.ts" />
+/// <reference path="njs_core.d.ts" />
--- a/ts/njs_shell.d.ts
+++ b/ts/njs_shell.d.ts
@@ -1,4 +1,4 @@
-/// <reference path="index.d.ts" />
+/// <reference path="njs_core.d.ts" />

I’ve changed references to index.d.ts so the devs using njs-types don’t have to reference two files: one of the entrypoints plus njs_core.d.ts.

BTW, why did you remove/didn’t include README.md? This is for npmjs.com, it automatically reads README.md from the package shows it as the package’s description.

I forgot that you have to take the patch and apply it on the mercurial repository, so you cannot easily sync the changes. I should probably stop rebasing commits and just add fixups instead… I’m gonna do it for the last time now just to sync your changes back to this PR. :)

Was the removal of null in NginxHeadersOut intentional? I’m not sure if it should accept it or not.

With strict type checking options enabled (they are recommended to be enabled in all new TypeScript projects).

To prevent developers from accidentally committing them into the repository.

ts_test was changed to work as an integration test - it packs njs-types into a package, installs it in test/ts as a dependency, installs typescript from npm registry and then checks types in the test files using the installed tsc. The goal is to verify a typical usage scenario of the njs-types package, including the project's configuration.

Fixes the following type errors: Property ''xxx'' of type 'NjsByteString | undefined' is not assignable to string index type 'NjsStringLike'.ts(2411) https://www.typescriptlang.org/docs/handbook/interfaces.html#indexable-types: > While string index signatures are a powerful way to describe the > "dictionary" pattern, they also **enforce that all properties match > their return type.**

xeioex · 2020-10-26T20:41:50Z

@jirutka

BTW, why did you remove/didn’t include README.md? This is for npmjs.com, it automatically reads README.md from the package shows it as the package’s description.

it is in progress, I did not get to README.md patch yet (plan to add).

Was the removal of null in NginxHeadersOut intentional? I’m not sure if it should accept it or not.

yes, I plan to simply drop the test which caused the issue.

The meaning of reordering is to reduce the patches size (by squashing some changes).

jirutka · 2020-10-26T20:47:34Z

yes, I plan to simply drop the test which caused the issue.

Okay, I’ve synced this change back to this PR. And that was the last rebase.

The meaning of reordering is to reduce the patches size (by squashing some changes).

Yeah, I understand that.

xeioex · 2020-10-27T18:46:21Z

@jirutka

Here is my current patchset
https://gist.github.com/xeioex/f62ea58fabb6022359c20e7f28638660

Remaining problems are:

ts/*.json ts/*.d.ts ts/**/*.d.ts it does not look like a portable make deps syntax (It looks like a globstar bash extention https://unix.stackexchange.com/questions/117826/bash-globstar-matching/315116).
moreover make ts_publish NJS_TYPES_VER=0.4.2

may not work as expected, if make ts_test was already invoked with different NJS_TYPES_VER value.
Because $NJS_BUILD_DIR/ts/package.json does not explicitly depend on NJS_TYPES_VER.

NJS_TYPES_VER dependency can be introduced, but maybe the dump targets (no dependencies) are a better option for this very specific task.

jirutka · 2020-10-27T21:59:01Z

The patchset is broken because there’s missing ts/index.d.ts.

# auto/make:
-$NJS_BUILD_DIR/test/ts/package.json: test/ts/*.json test/ts/*.ts
+$NJS_BUILD_DIR/test/ts/package.json: ts/*.json ts/*.d.ts ts/**/*.d.ts
        mkdir -p $NJS_BUILD_DIR/test
        cp -fr test/ts $NJS_BUILD_DIR/test/

There should be test/ts/*.json test/ts/*.ts, not ts/*.json ts/*.d.ts ts/**/*.d.ts – this target copies ./test/ts into ./build/test/ts, not ./ts.

# test/ts/tsconfig.json:
         "strictPropertyInitialization": true,
-        "noImplicitThis": true,
+          "noImplicitThis": true,
         "alwaysStrict": true,

There are two extra spaces.

Just a suggestion (I find the current indentation confusing):

# auto/make:
$NJS_BUILD_DIR/test/ts/node_modules: \\
-       $NJS_BUILD_DIR/njs-types-\$(NJS_TYPES_VER).tgz \\
-       $NJS_BUILD_DIR/test/ts/package.json
+               $NJS_BUILD_DIR/njs-types-\$(NJS_TYPES_VER).tgz \\
+               $NJS_BUILD_DIR/test/ts/package.json
       cd $NJS_BUILD_DIR/test/ts && \$(NPM) install \\
-       --save-dev file:../../njs-types-\$(NJS_TYPES_VER).tgz
+               --save-dev file:../../njs-types-\$(NJS_TYPES_VER).tgz
       cd $NJS_BUILD_DIR/test/ts && \$(NPM) install

ts/.json ts/.d.ts ts/**/*.d.ts it does not look like a portable make deps syntax (It looks like a globstar bash extention

I don’t have that much experience with writing (portable) Makefiles to disquish between POSIX make and GNU make.
What about his approach? https://github.com/nginx/njs/commit/3f407ce2e4d343ad11d1437b2a1d1415fd12f4be.patch

moreover make ts_publish NJS_TYPES_VER=0.4.2
may not work as expected, if make ts_test was already invoked with different NJS_TYPES_VER value.
Because $NJS_BUILD_DIR/ts/package.json does not explicitly depend on NJS_TYPES_VER.

You’re right. Here is a fix: https://github.com/nginx/njs/commit/4412267a015d5ecdf309cae004f98db4afd19742.patch

xeioex · 2020-10-28T12:44:49Z

@jirutka

You may also help us in improving the development guide for the end users (possibly for newcomers to the node.js world).
See http://nginx.org/en/docs/njs/typescript.html.

I am not a JS nor node.js developer. So best practices and good examples for writing njs code using njs-types are welcome.

Here is my latest patchset: https://gist.github.com/e94a527711e04011c616ccc36025e540

For example, what is the proper command to produce JS artefact using npm? (analogous to tsc test.ts)

jirutka · 2020-10-28T16:50:05Z

You may also help us in improving the development guide for the end users (possibly for newcomers to the node.js world). See http://nginx.org/en/docs/njs/typescript.html.

I’ll look into it after I actually finish the setup of my project using njs.

For example, what is the proper command to produce JS artefact using npm? (analogous to tsc test.ts)

You just declare it as the build script in package.json, e.g.:

{ ...
  "scripts": {
    "build": "tsc test.ts"
  },
  ...
}

and then run npm build.

BTW, tsc in test/ts/ now does not produce test.js (which we don’t need here) due to "noEmit": true in tsconfig.json.

Here is my latest patchset: https://gist.github.com/e94a527711e04011c616ccc36025e540

There is still ts/index.d.ts missing so it doesn’t work. It should be included in commit Types: refactored working with TypeScript API description..

Introduced .hgignore.

This commit doesn’t introduce just .hgignore file, but also .gitignore, so it should be named Introduced .hgignore and .gitignore. Speaking of which, it would be good to ignore /build and /Makefile as well:

# /.hgignore:
+ ^build/
+ ^Makefile$

# /.gitignore
+ /build/
+ /Makefile

Types: added missing var "console" into njs_shell.

To be perfectly correct, this commit should come after Types: added tsconfig.json and package.json for type descriptions. because the default typescript config (used when tsconfig.json is not present) includes the browser environment where console is already defined which results in error.

+ NJS_TEST_TS_SRCS=$(find test/ts/ -name "*.d.ts" -o -name "*.json")

This is incorrect, test/ts/ currently doesn’t contain any *.d.ts files, but *.ts files (just one actually, test.ts).

If you just wanted to avoid pipe in this subshell, you can also write it as $(find test/ts/ -name "*.ts" -o -name "*.json" -exec echo {} +) to produce a space-separated (instead of newline-separated) filenames which can be directly used as dependencies in Makefile.

jirutka · 2020-10-29T11:37:48Z

I’ve updated compilerOptions.lib in tsconfig.json to match njs compatibility:
https://github.com/nginx/njs/pull/344/commits/708c204d57ba79691e1438eeb0cd052600cb5f35.patch

xeioex · 2020-10-29T12:34:54Z

@jirutka

If you just wanted to avoid pipe in this subshell, you can also write it as $(find test/ts/ -name ".ts" -o -name ".json" -exec echo {} +) to produce a space-separated (instead of newline-separated) filenames which can be directly used as dependencies in Makefile.

Actually, this is not needed, because newlines are replaced with spaces in the second command substitution (for njs_test_ts_deps).

xeioex · 2020-10-29T13:01:43Z

@jirutka
Thank for your contribution. Committed.

jirutka · 2020-10-29T14:08:37Z

You’re welcome!

Why did you release njs-types 0.5.0? The njs-types version should be aligned with the njs version and there’s no 0.5.0 yet. The latest released njs version is 0.4.4 and the types currently reflects this version, so you should release njs-types 0.4.4.
For the pre-release / development versions, use a standard pre-release suffix in format -<sufix>.<num>, e.g. -dev.1, -pre.1, -alpha.1 etc.
Numeric-only pre-release suffix (-2) is valid per SemVer, but it’s generally not recommended – it has unclear semantic. I’ve suggested it just as a workaround explained here, but eventually dropped it as a bad idea.

xeioex · 2020-10-30T09:40:39Z

@jirutka

Why did you release njs-types 0.5.0? The njs-types version should be aligned with the njs version and there’s no 0.5.0 yet.

This is my bad. I accidentally did make ts_publish. I think defining NJS_TYPES_VER by default, was not the best idea. ts_publish should check whether NJS_TYPES_VER is defined, and warn the user if it is not.

jirutka · 2020-10-30T10:29:28Z

I think defining NJS_TYPES_VER by default, was not the best idea.

Yeah, it looks like that.

jirutka · 2020-10-30T10:34:14Z

Merged in 07f3d66..fc08c95.

xeioex · 2020-11-02T14:27:29Z

@jirutka

Yeah, it looks like that.

Take a look https://gist.github.com/xeioex/95fe47ae0fce376e18de06674e6dbe50.

jirutka · 2020-11-02T22:42:01Z

Take a look https://gist.github.com/xeioex/95fe47ae0fce376e18de06674e6dbe50.

This is not good, it would require providing NJS_TYPES_VER even for running ts_test.

jirutka force-pushed the njs-types-pkg branch 2 times, most recently from 3a6012f to 2862f15 Compare October 20, 2020 19:35

jirutka mentioned this pull request Oct 20, 2020

Add TypeScript types for modules #342

Closed

jirutka force-pushed the njs-types-pkg branch 2 times, most recently from 5affa01 to f63e61b Compare October 20, 2020 20:17

jirutka force-pushed the njs-types-pkg branch from d4a8571 to 611943a Compare October 21, 2020 14:01

jirutka force-pushed the njs-types-pkg branch 2 times, most recently from 5dd1b34 to 3422eb1 Compare October 22, 2020 12:23

jirutka commented Oct 22, 2020

View reviewed changes

ts/ngx_http_js_module.d.ts Show resolved Hide resolved

jirutka force-pushed the njs-types-pkg branch from 662fda5 to 4c38953 Compare October 22, 2020 17:44

Types: moved all TS type descriptions to ts/ in the repo root.

4559ed6

jirutka force-pushed the njs-types-pkg branch from 4c38953 to 6e8db86 Compare October 22, 2020 20:54

jirutka changed the title ~~WIP: Prepare TS types for publishing on npm~~ Prepare TS types for publishing on npm Oct 23, 2020

jirutka force-pushed the njs-types-pkg branch 2 times, most recently from 37d7ff7 to ee5de8b Compare October 24, 2020 19:54

jirutka force-pushed the njs-types-pkg branch 3 times, most recently from 5b6481b to 54ed7a4 Compare October 25, 2020 09:48

xeioex added the docs label Oct 26, 2020

jirutka added 3 commits October 26, 2020 21:35

Types: added tsconfig.json and package.json for type descriptions.

4539713

With strict type checking options enabled (they are recommended to be enabled in all new TypeScript projects).

Added node_modules/ and package-lock.json into .hgignore/.gitignore.

0b4ca92

To prevent developers from accidentally committing them into the repository.

Make: updated ts task and added ts_lint and ts_publish tasks.

1505741

jirutka added 4 commits October 26, 2020 21:35

Types: added tsconfig.json and package.json to test/ts.

83fdea0

Added COMMITHASH into njs-types package.

fa9d234

jirutka force-pushed the njs-types-pkg branch from 54ed7a4 to 5fda5df Compare October 26, 2020 20:36

jirutka added 3 commits October 26, 2020 21:45

Types: fixed TS errors in test.ts with strict checks enabled.

15a67d2

Types: added missing var "console" into njs_shell.

c378622

Types: added Readme for npmjs to njs-types.

0206555

jirutka force-pushed the njs-types-pkg branch from 5fda5df to 0206555 Compare October 26, 2020 20:45

jirutka added 2 commits October 27, 2020 22:16

fixup: avoid using globstar bash extention in make

3f407ce

fixup: fixed problem with ts_publish and NJS_TYPES_VER

4412267

xeioex added the types label Oct 28, 2020

fixup: fixed compilerOptions.libs in tsconfig.json.

708c204

jirutka closed this Oct 30, 2020

jirutka deleted the njs-types-pkg branch October 30, 2020 10:34

Prepare TS types for publishing on npm #344

Prepare TS types for publishing on npm #344

Uh oh!

Conversation

jirutka commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xeioex commented Oct 21, 2020

Uh oh!

jirutka commented Oct 21, 2020

Uh oh!

jirutka commented Oct 21, 2020

Uh oh!

xeioex commented Oct 21, 2020

Uh oh!

jirutka commented Oct 22, 2020

Uh oh!

Uh oh!

jirutka commented Oct 24, 2020

Uh oh!

xeioex commented Oct 26, 2020

Uh oh!

jirutka commented Oct 26, 2020

Uh oh!

xeioex commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jirutka commented Oct 26, 2020

Uh oh!

xeioex commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jirutka commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xeioex commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jirutka commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jirutka commented Oct 29, 2020

Uh oh!

xeioex commented Oct 29, 2020

Uh oh!

xeioex commented Oct 29, 2020

Uh oh!

jirutka commented Oct 29, 2020

Uh oh!

xeioex commented Oct 30, 2020

Uh oh!

jirutka commented Oct 30, 2020

Uh oh!

jirutka commented Oct 30, 2020

Uh oh!

xeioex commented Nov 2, 2020

Uh oh!

jirutka commented Nov 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jirutka commented Oct 20, 2020 •

edited

Loading

xeioex commented Oct 26, 2020 •

edited

Loading

xeioex commented Oct 27, 2020 •

edited

Loading

jirutka commented Oct 27, 2020 •

edited

Loading

xeioex commented Oct 28, 2020 •

edited

Loading

jirutka commented Oct 28, 2020 •

edited

Loading