Testing core-decorators.js using TypeScript

[BurtHarris]

I'm working on a branch with the idea of using TypeScript tsc to build core-decorators.js and the tests. Progress good, but one issue has me puzzled. When I code like import decorators from '../'; I'm getting a node.js error Error: Cannot find module '../'.

I'm not sure how this works under babel, I thought there had to be an index.js in the directory to make this work.

https://github.com/BurtHarris/core-decorators.js/tree/typescriptTests if you want to have a peek.

[BurtHarris]

Oh, I see, it's the "main" in package.json that makes this work... Hmm.

[jayphelps]

Since the tests run under node, they use the node resolution process (it also uses babel-core/register but that doesn't impact file resolution AFAIK).

The main in package.json points to the correct entry point, which is how this works.

The reason this is done is to make sure the tests are testing the same thing our users will use. I've seen cases where projects import directly and miss bugs that users hit, like something not being exported properly, etc.

[BurtHarris]

OK, I guess to make that work I'll have to put the TypeScript outputs into 'lib' instead of something typescript specific. I've run into some other interesting issues at compile time:

src/override.js(113,42): error TS8010: 'types' can only be used in a .ts file.
src/override.js(113,59): error TS8010: 'types' can only be used in a .ts file.
test/unit/mixin.spec.js(22,10): error TS1109: Expression expected.
test/unit/mixin.spec.js(37,17): error TS1109: Expression expected.

The first two are because there's TyepScript style typing in a JavaScript file. I'm assuming can comment that out -- short term.

The later two are because TypeScript doesn't support decorators on class expressions. Is there some way to avoid that in the mixins decorator test? TypeScript now supports mixins natively, so perhaps that test isn't important.

[jayphelps]

@BurtHarris hmm what's the end goal of this btw? to PR this so that the tests run under TypeScript I assume? There is some known issue with TypeScript and autobind #86 that I haven't had time to figure out. This would be neat to use to figure it out!

re the override decorator having a type--that is an accident. I imagine I was just so used to giving type signatures in other projects that I typed it in my sleep. Can be removed since it's not actually checked anyway.

re mixins, I think the test can easily be refactored to not need the decorator on a class expression at the expense of more duplication but not a huge deal.

[BurtHarris]

@jayphelps I'd like to see good TypeScript support in the package,* without requiring TypeScript to be used by calling code.

As background, I'm considering using core-decorators to replace some stubbed-out decorators in my antlr4ts project, which is written in TypeScript. But I'm nervous as the core-decorates docs say it's not officially supported but doesn't get specific about the incompatibilities. So the more specific point my experiment is to get an accurate inventory of bugs impacting use of core-decorators from TypeScript, and perhaps fix them where it doesn't hurt the babel version.

At this point think TypeScript is closer claiming close conformance with the stage 2 proposal. https://www.typescriptlang.org/docs/handbook/decorators.html

* Note that I'm not saying the end goal is to port core-decorators to TypeScript files, that's your call however IMHO it wouldn't be a bad idea. It should be a simple matter to run tests of
babble-generated callers running against the TypeScript-generated library, and so far that's worked even when I need to tweak the sources.

[jayphelps]

@BurtHarris all sounds good, except I want to make a critical distinction: I'm pretty confident TypeScript supports stage-0 decorators, just the same as core-decorators does.

They have this quote:

Decorators are a stage 2 proposal for JavaScript and are available as an experimental feature of TypeScript.

But it's very misleading! It isn't saying TypeScript implements the stage-2 spec, it's just saying that "decorators" as a thing has reached stage-2. The spec changed very incompatibly between stage-0 and stage-2, and no compiler that I am aware of has implemented the stage-2 spec.

The easiest way to know which spec they implement is to look at the arguments that the decorator is provided by the JS runtime. It varies depending on class vs property (and getter/setters) but let's just look at properties since it will make it obvious:

In the case of stage-0 (and stage-1, I believe, but haven't confirmed) it is provided the target, property key as a string, then the property descriptor itself:

function (target, key: string, descriptor: PropertyDescriptor);

interface PropertyDescriptor {
  configurable: boolean,
  enumerable: boolean,
  value: any,
  writable: boolean
}

But in stage-2, it was changed to a single argument of a new, special "MemberDesciptor":

function (descriptor: MemberDesciptor);

interface MemberDesciptor {
  kind: "Property"
  key: string,
  isStatic: boolean,
  descriptor: PropertyDescriptor,
  extras?: MemberDescriptor[]
  finisher?: (klass): void;
}

There are other differences, but this is the easiest to quickly see which version you're working with.

TypeScript implements the original, stage-0 spec and I haven't seen any public work to implement the stage-2 spec, probably because so many people depend on stage-0 at this point that they're going to wait for stage-3 (or 4) spec before breaking everyone.

[jayphelps]

the latest issue with autobind and TypeScript is most likely related to the way they polyfill classes being different than babel, rather than differences in the decorator implementations. That's just a guess though, cause that was the case of previous issues I resolved with TypeScript vs Babel decorators.

[BurtHarris]

You're right about stage-0 vs stage-2. I misunderstood the document and agree the wording is poor. I also haven't been tracking the changes closely, so I appreciate the info.

I'll put a few more licks in on the experiment tonight, but so far biggest change I found needed wsa places where the tests were doing default imports, e.g. import chai from 'chai', which needed to change to namespace imports, e.g. import * as chai from 'chai' for the test to work with TypeScript.

One odd thing is running the tests with babble, either sort of import works.

I know there's a reason TypeScript implements default imports differently, but only vaguely remember the reason was unpleasant at the time I read it...

[jayphelps]

hmm that's odd indeed. Generally the emitted runtime code looks to same for imports/exports AFAICT. I'm guessing it's a type thing maybe? Are you including @types/chai?

I'm not super familiar with the namespace feature, so it's unclear whether this includes an actual default export or not https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/chai/index.d.ts

[BurtHarris]

Ahh, I had included @types/mocha but not @types/chai, not sure that will help though. I found the codegen difference which explains the asymmetrical results:

import * as chai from 'chai'
const should = chai.should();

tsc generates something unsurprising...:

var chai = require("chai");
var should = chai.should();

however for:

import chai from 'chai'
const should = chai.should();

tsc generates:

var chai_1 = require("chai");
var should = chai_1.default.should();

Obviously the .default on the last line fails at runtime. The difference seems to be that babel is a fallback for modules without the _esModule property defined which adds a wrapper object with .default defined.:

var _chai = require('chai');
var _chai2 = _interopRequireDefault(_chai);
function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }
var should = _chai2.default.should();

There are a number of TypeScript bugs dealing with this, but they seem to have been closed. For example: microsoft/TypeScript#11179.

[jayphelps]

Wow they don't check for __esModule interop?? They even emit the __esModule: true code on their exports! That's silly. Your * as chai solution sounds good and has the same effect for both. Thank you!

[jayphelps]

@BurtHarris it appears (but I haven't confirmed) that in TS the allowSyntheticDefaultImports option is needed to get the same CJS "default" import fallback as babel.

[BurtHarris]

Oh, snap! Seems relevant, but I may have been overoptimistic @jayphelps. The codegen from ts with that enabled still doesn't check the __esModule flag when importing, it still seems to generate:

var chai_1 = require("chai");
var should = chai_1.default.should();

Pretty sure that still won't work for chai.

One other pain-in-the-*** I wanted to run by you has to do with the structure of the output directory (e.g. lib):

When I include just the src directory in a TypeScript compilation unit, the output directory gets the files as expected. However if I try to build the test directory (even alone), the references are followed by the compiler and it ends up structuring the lib directory differently, so lib/test and lib/src contain the files. Without intervention, this extra level of nesting of the output test JavaScript files causes them to fail at runtime because the package.json isn't in lib, generating an error on statements like require("../"), so it can't find core-decorators.js.

I've bumped my head on this before, but never settled on a solution I think is "good". For now, I think the simplest thing is to tell the TypeScript compiler to output to a built directory, then as a post-compile step copy the files into the directory structure needed to make the tests run. But it feels rather like a kludge.

An alternative might be to change the main etc in package.json to reflect the location lib/src, but then I'd wan't to have babel build it that way too. Seems too complicated.

[BurtHarris]

One other way of addressing the output directory structure issue could be to move core-decorators.js into the project root directory, and rename it to index.js. Then package.json wouldn't need to be in the directory above the built test, and the relative reference in the tests would work without it. Some details to work out, but as I'm writing this note, it seems promising, what do you think?

[BurtHarris]

I opened a TypeScript issue on the __esModule flag: microsoft/TypeScript#16090.

[jayphelps]

Very nice!! I see they even responded and even discussed in their latest meeting.

[BurtHarris]

@jayphelps, I've learned some interesting stuff about build tools that might be applicable. Gulp and Lerna both seem attractive and potentially complementary tools. Lerna addresses having multiple different multiple npm packages in a single repository, coordinating version numbers, etc. Might be a good way of managing having both a TypeScript and Babel builds until the compatibility issues get worked out.

While not a master of either, I think I could PR a change to the build process to use them.

[jayphelps]

@BurtHarris hmm I definitely don't want to maintain two versions, one in TS and one in babel. 😢

[BurtHarris]

@jayphelps to be clear, I was thinking that both versions would be build from the same source code, but I understand the reluctance.

[jayphelps]

@BurtHarris what all is left to merge the index.d.ts with the actual source files so we don't need to maintain both? i.e. build from TS which will generate the actual definitions.

[BurtHarris]

@jayphelps, I'll re-examine at that today. If I remember correctly the tests I had the hardest time with were ones for the deprecated decorators: @debounce, @throttle and @mixin. If I predicate it with removal of those (per our discussion on #127), I think it's not a lot of work.

I've learned a trick or two in the meantime... :-)

[BurtHarris]

Oh yea, this packages @memoize was somewhere listed as a deprecated features too..., wasn't it? I'm assuming that can go now too, but that we continue to support and test @decorate using lodash's memoize. @jayphelps please confirm.

[BurtHarris]

@jayphelps, I'm a but unclear on what should go into the es directory, should it target es2015, or es2016, or something else?

[jayphelps]

@BurtHarris the es directory is supposed to contain ES5.1 code except using ES modules instead of CJS. If I were to redo it today, I would have named the directory esm as I think that has become a somewhat prominent abbreviation for ES Modules syntax + ES5.1 code

[BurtHarris]

@jayphelps You're at the edge of my understanding here, but I'm not sure that TypeScript really supports the combination output you are describing, though I have to admit some troubleshooting on an unrelated issue make it sound like a useful one.)

If I use --target es2015, then TypeScript gets close, but it may also include other es2015 features, particularly class constructs, which seems to be at the root of a lot of 'learning' (stalled) time recently. I just figured out it it might be possible to do what you are suggesting using --module es2015. Does that sound right?

Searching the web for material similar to what you were saying I found this: https://github.com/systemjs/systemjs/blob/master/docs/module-formats.md. I've not looked into SystemJS much, is this what you are referring to?

[jayphelps]

@BurtHarris sorry for the delayed response, did you figure it out?

Here's an example taken from an internal library I have that does this:

    "build": "npm run build:es2015 && npm run build:esm && npm run build:cjs && npm run build:umd && npm run build:umd:min",
    "build:es2015": "tsc --module es2015 --target es2015 --outDir dist/es2015",
    "build:esm": "tsc --module es2015 --target es5 --outDir dist/esm",
    "build:cjs": "tsc --module commonjs --target es5 --outDir dist/cjs",
    "build:umd": "rollup dist/esm/index.js --format umd --name Wazzup --sourceMap --output dist/umd/wazzup.js",
    "build:umd:min": "cd dist/umd && uglifyjs --compress --mangle --source-map --screw-ie8 --comments --o wazzup.min.js -- wazzup.js && gzip wazzup.min.js -c > wazzup.min.js.gz",

[BurtHarris]

Yea, I did. There are some fine points about the differences betwee --target, --module, and --lib I hadn't understood before. Good learning experience.