Improved typings compatibility

[russaa]

ZeroMQ.js version: 6.0.0-beta.6

with current configuration, typescript v3.7.x is used for compiling which introduces backwards-incompatible typings for properties

this PR

• uses typescript v3.6.x for compiling provides backwards compatible typings for typescript < 3.6.x via fields types and typesVersions in package.json:
  backwards compatible typings avoid typing errors error TS1086: An accessor cannot be declared in an ambient context. when zeromq.js is used in projects that use typescript < v3.6.0
• simplifies type definitions for AsyncIterator in order to increase typing compatibility:
  it uses simplified typings for AsyncIterator in the backwards compatible typings, i.e. when referenced by projects that use typescript < v3.6.0 (see above) the "simple" type definition of AsyncIterator of the esnext lib with one instead of two generics parameters is used (the typgings for >= 3.6.0 still use the typings definition of AsyncIterator according to es2018)

the backwards compatible typings are generated with the script "script/ci/downlevel-dts.js" (included in this PR)

comparing the compiled results between v3.7.3 and v3.6.4, there were no differences in the generated JavaScript code, only in the generated typings, i.e. in the *.d.ts files, so currently, for the actual executable code, it makes no difference to downgrade to typescript v3.6.x

the drawback for the 2nd change regarding AsyncIterator is that the typing becomes less restrictive for target es2018, changing the TReturn type from undefined to any, but I think that in this context it is justifiable when weighing it against increased typings compatibility

[russaa]

... or define multiple typings in package.json via typesVersions, but as far as I understand it, this would require installing/using multiple typescript versions, since there are no compiler options (that I know about) to create typings for an older typescript version

[rolftimmermans]

I am not in favour of downgrading the TypeScript version, because it will prevent benefiting from bug-fixes and newer features as they are released in future releases.

I'd be much happier if we can output different typings for older TS versions using typesVersions. I have been looking at https://github.com/sandersn/downlevel-dts for that, but I couldn't get it to work yet unfortunately.

[russaa]

OK -- if you want, I can take a look at https://github.com/sandersn/downlevel-dts and open a new PR when I get it to work (or another solution for utilizing typesVersions)

[russaa]

... fyi:
I got it to work using the fork https://github.com/dsherret/downlevel-dts (branch issue1)

• create dir & file ts3.5/tsconfig.json with contents:

{
  "extends": "../tsconfig-build.json",
  "include": ["../lib"],
  "compilerOptions": {
    "outDir": "./"
  }
}

then run

npm install git+git+https://github.com/dsherret/downlevel-dts.git#issue1 --save-dev
node -e require('demake-dts') demake-ts ./ ts3.5

but there is almost no configuration possible, e.g. for name/path of tsconfig.json file, source-files etc.
For example, it will always take all existing *.d.ts files and modify them, so running it twice will process the previously created *.d.ts files and emit them to a sub-directory.

Also: this will only fix/modify the the typings for exactly this known property/accessor problem

that is: this would be no general solution for backwards compatibility, but only for this specific breaking change

[rolftimmermans]

It might be better for now to just include a custom TS script that does the transformation, similar to the approach here: https://github.com/MattiasBuelens/web-streams-polyfill/blob/779bd151a8d9f457406c4a79c625eb872d480a1c/build/downlevel-dts.js.

And when downlevel-dts is mature enough to be configurable, we can replace the custom script.

[russaa]

looks good:

I updated the pull request by

• restoring the original typescript version in package.json
• using the linked script web-streams-polyfill/build/downlevel-dts.js for creating backwards compatible typings in /lib and the latest/unmodified typings in /lib/ts3.6
• adding a corresponding typeVersions entry in package.json

[rolftimmermans]

Looks great! The downlevelTS34 function doesn't do anything for the typings of these project, does it? Might be better to remove it if it's unused...

Next question is how we can make sure that the typings can be tested, of course. I'd imagine we would need a separate test run of some kind. If you have any ideas that would be great!

Did you try the end result? Do you know what the minimum TypeScript version is for the modified typings?

[russaa]

I checked the results:
the (re-build) package works in projects that use typescript v3.5.3 themselves -- and will emit tsc build errors if the typings are overwritten with the ones from lib/ts3.6 again (i.e. the >= 3.7 typings)

I do not know, how far down the typings are compatible now, but I could look into it.

And yes, downlevelTS34() does not have any effect for now -- it seems to be meant to deal with changes of the asynciterable interface:
I did some manual FIXES w.r.t. this (i.e. concerning changes from esnext to es2018; see details of initial PR comment above), but these are still necessary, i.e. if reverted & re-compiled & processed by the downlevel-dts.js script, the resulting typings are not compatible if used in projects that do not target/include the es2018 typings.

As for testing:
there does not seem to be straight forward way with mocha (and ts-node) to specify a specific version of typescript that is different from the main project.
Other than creating a sub-project with a separate test (and typescript version), I do not have any great ideas, on how to test for the typings compatibility.

So asking back:

1. should I check, how "far down" these typings are compatible now?
2. should I remove downlevelTS34()?
3. should I add a test via a sub-project?

[rolftimmermans]

I think if downlevelTS34 is not used then we should remove it or (in an ideal world) they do the transformations that you did manually (but maybe that's a stretch).

With regards to testing, right now we have test/unit/typings-test.ts which is included in the mocha test suite and refers to src (../../src). Of course it should refer to the project root if we want to test multiple versions, so the typesVersions field can be picked up and it uses the lib/*.d.ts files instead of src/*.ts. I think we can just move it from test/unit to test, and include some new test target for Travis. The test script could just be something like tsc --noEmit test/typings-test.ts or yarn add --dev typescript@3.4 && tsc --noEmit test/typings-test.ts for any previous version. As long as it's isolated in a single CI task it doesn't affect the rest of the test suite. Does that make sense?

It would be good to have a minimum version requirement in the README. Once we can easily run the tests (see above) then I don't really see why we can't just add 10 test targets for all semver-minor TS versions and see which ones break. :-)

[russaa]

good!
I'll look into it & get back
(but probably not before next year)

[russaa]

I added typings-compatibility test:

basically the test copies the existing test case file test/unit/typings-test.ts into a temporary project (sub-directory), installs a typescript version, and then compiles it

I have created test cases for typescript versions 3.0.x - 3.7.x (i.e. incrementing the minor version number) -- with 2.9.x or lower it did not compile, or at least not with the typings as they are now.

See also the corresponding test case array in test/unit/typings-compatibility-test.ts

For 3.0.x and 3.1.x there are additional requirements:
either setting the typescript compile target to at least esnext, or adding the following libraries to the lib option: es2015, ESNext.AsyncIterable

For the later versions, i.e. starting with 3.2.x, the target can be set to es3

[russaa]

... something strange is happening:

on Windows the test cases for 3.2.x - 3.4.x run successfully for min. target es3, but on *nix (testing with Ubuntu), they fail due to the same cause as 3.0.x - 3.1.x

I.e. changing the targetto esnext, or using target es3 with lib es2015 and ESNext.AsyncIterable compiles successfully

(I am using node version 11.15.0 on both systems)

I have no idea, why there should be a difference when running this on Windows and on *nix in this case!?!

Do you have any idea what could cause this?

... or should I just change the test cases, i.e. the "minimal requirements" for using the library with those typescript versions?

[russaa]

Update:
I did change the test-cases for 3.2.x - 3.4.x to conform to the *nix requirements, i.e. either minimal target exnext or es3 with additional (minimal) libraries es2015 and ESNext.AsyncIterable.

The release notes for typescript v3.6 indicate that the required typings for the AsyncIterator are included by default since then -- and I guess they were already included in version 3.5.

The real question is, why typescript compiles on Windows even for typescript versions 3.1.x - 3.4.x without complaining ... it's probably a bug

[russaa]

... the test fails on one vm because npm install cannot be executed in a child-process, which is used in the before-hook for typings-compatibility tests to set up the targeted typescript version

the error message is

Error: Command failed: npm install

/bin/sh: npm: not found
      at ChildProcess.exithandler (child_process.js:294:12)
      at maybeClose (internal/child_process.js:982:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)

so npm is not in the global path here(?)

[rolftimmermans]

Great job so far! Looks like we're almost there?

I know Yarn allows multiple versions with aliases, like this yarn add --dev typescript36@npm:typescript@3.6. So you could install every version upfront and just use whichever one is needed. But it will definitely break compatibility with NPM for development, so that's probably too harsh on contributors.

Perhaps it's possible to vendor the typescript dependencies? I don't think typescript itself has any dependencies so that might not be very difficult.

[russaa]

yes, I think it's almost there

I still have problems understanding the reason for the specific vm setup:
all the other vms do have node and npm installed "normally", so that they can be invoked in the shell by their command.

... except for this vm: is there a special reason that this specific setup (with no access to node and/or npm command) needs to be tested?

Also looking into the details:
it seems that initially node version v0.10.48 (along with npm v2.15.1) is installed, but during the setup for alpine linux a package for node v10.16.3-r0 is installed and then used in the compilation process for zeromq.js ... why not start out with that version of node?
(is it maybe a typo that nvm installs 0.10.x instead of 10.x.x?)

Anyway:
it looks to me as if the test environment (node version = 10.16.3 & os = linux & architecture = x64) is already covered in other vms, except for the non-availability of npm -- or am I overlooking something?

In that case, I think, we do not really need to run the typings compatibility test on that specific vm.
So I would suggest to make the test disable'able via an environment variable & start that vm with the typings compatibility test disabled.

Do you think, that would be acceptable, or needs this test to work in that vm setup too?

[rolftimmermans]

I think it's okay to disable the versioned typings tests via an environment variable for this particular test run. It's there specifically to test Alpine Linux which runs a different libc (musl instead of glibc).

[russaa]

OK, great -- it should work now

I added an environment variable EXCLUDE_TYPINGS_COMPAT_TESTS:
if set to true the compatibility tests will be skipped.

... but:
I ended up changing the test so that it detects & runs either via npm install or yarn install, so it will even work in the alpine environment now

---

the reason being that for some reason setting the environment variable (via .travis.yml) did not work on the alpine vm either:
it did not set it in a way recognizable by node, i.e. it was, for some reason, not set in process.env.EXCLUDE_TYPINGS_COMPAT_TESTS when running the test suit on the alpine vm

[rolftimmermans]

Yeah not all environment variables are forwarded by default, you'd have to match the names with the prefixes in https://github.com/zeromq/zeromq.js/blob/master/script/ci/install.sh#L17.

[rolftimmermans]

One job failed but after a restart everything seems fine. Is it ready to merge do you think?

[russaa]

in principle, I think it's ready

although it may not be a bad idea, to add something to the README w.r.t. the supported typescript versions and required compilerOptions.targets and/or compilerOptions.libs

should I add a draft for this to the README?

[rolftimmermans]

That would be great! I agree more documentation would be helpful.

[russaa]

OK, I added some description & requirements for use with TypeScript in the Examples section -- I am not completely sure where the best location would be ... maybe the Installation section would be better suited(?)

But, if you think it's fine, then I would say the PR is ready to be merged