Typescript build failing tests. (@lazy-initialize not working correctly with tsc)
Opened this issue · 17 comments
I've been going through some of the typescript features that you've been adding into the project and noticed in issue #123 that after adding in the compiler step that 20 of the tests were failing.
I've narrowed it down to a problem with lazy-initialize being used in the Meta constructor in the private/utils file.
For some reason Meta is not being called during the loading of certain decorators, although funnily enough it still passes it's own tests. I'll keep looking into this when I have time but if you can think why this might be happening I'm all ears.
Removing the decorator from the Meta class allows all 73 tests to pass.
Reproduced same, I'm going to try to understand what's going on, but your workaround seems reasonable. With the obsolete features removed, the Meta class only has one property, removing its lazy initiialization seems reasonable, but I'm wondering if the tests for @lazyInitialize are covering the expected behavior.
Been a while but after a bit of digging around it appears that there is something weird going on with the way typescript is desugaring the decorator, I haven't pin pointed it down exactly but during debugging I've noticed that the property descriptor value is being passed in as undefined for those meta properties when transpiling using typescript.
There is also the wider issue here that the mocha testing environment is executing all of the decorators after transpiling using babel, the testing suite is defined to transpile on the line:
"npm run build && mocha --compilers js:babel-core/register --require babel-polyfill \"test/**/*.spec.js\"
within the package. I think the reason we aren't seeing the lazy-initialise issue during it's unit tests is because it is being transpiled in the file by babel, which we know works. However in the case of the Meta constructor failing it is actually first transpiled by typescript before being imported. I think we will need a new testing script to run the testing suite after it is transpiled by TS, however...
@jayphelps you mention in issue #120 that you don't want to maintain essentially two projects, one for TS and the other for babel when it comes to the decorators. It seems at this point anyway that it will be pretty difficult to do avoid if you plan on supporting both TS and the ECMA communities since their transpile engines have different desugaring strategies for decorators.
@BurtHarris thoughts?
Not sure I understand, but I'll try to wrap my head around it. I think using babel-core/register for testing may hide important details. That's not to say we shouldn't test Babel-transpiled code calling into the library, but it probably makes sense to transpile & save the tests in using both tools.
I think the clear goal is that one set of .js files for the library should suffice. It would be nice if the tests are the same source-wise as well, but for a couple of reasons that may not be practical in the short term. For example: TypeScript won't transpile files with the .js extension right now. It can be made to type-check .js files, but it's more picky about what can be in a .js file than Babel is. Babel (as configured for this project) seems to silently ignore typing information in .js files, but TypeScript finds it objectionable.
@dtweedle you mentioned the TS and the ECMA communities. Do you really mean TS and the Babel communities? As far as I know, ECMA hasn't settled on decorators, so there are no native implementations, and I think eventually both transpilers will have to change some.
--
@BurtHarris
Sorry my comment made it sound like I was advocating maintaining two libraries, this isn't the case, it's only the compilation step that needs amending.
Typescript and babel both transpile the core-decorators functions similarly from what I can see, it's the actual processing of the decorator themselves (that is the @decorator part) that are handled differently and right now we only use babel as a test runner compiler during testing.
If you want to replace the babel compiler entirely with the typescript one instead you're going to have to run the test suite with tsc.
And yes sorry I meant Babel and TS, not ES.
OK good, I understand better now. Given your observation that the actual code emitted for @decorator differs, I'm saying we can't afford to run the tests only using tsc, but that we need both a tsc and Babel version of the tests.
Getting the tests to run under tsc has been harder than I thought, but I'll give it another push today, and specifically convert them so that the babel tests actually generate code so I can see and characterize the differences in codegen.
@dtweedle, @jayphelps: TypeScript currently has a documented restriction on property decorators:
NOTE A Property Descriptor is not provided as an argument to a property decorator due to how property decorators are initialized in TypeScript. This is because there is currently no mechanism to describe an instance property when defining members of a prototype, and no way to observe or modify the initializer for a property. As such, a property decorator can only be used to observe that a property of a specific name has been declared for a class.
I've now observed this side-by-side in calls to the nonenumerable decorator from both Babel and TypeScript, the typescript version gets undefined instead of the property descriptor. This input code in nonenumerable.spec.js:
class Foo {
@nonenumerable
bar = 'test';
}Gets translated by TypeScript to
var Foo = (function () {
function Foo() {
this.bar = 'test';
}
__decorate([
core_decorators_1.nonenumerable
], Foo.prototype, "bar", void 0);
return Foo;
}());The void 0 taking the place of the missing descriptor parameter... Babel on the other hand generates:
var Foo = (_class = function Foo() {
_classCallCheck(this, Foo);
_initDefineProp(this, 'bar', _descriptor, this);
}, (_descriptor = _applyDecoratedDescriptor(_class.prototype, 'bar', [_coreDecorators.nonenumerable], {
enumerable: true,
initializer: function initializer() {
return 'test';
}
})), _class);So the Babel transpiler creates a synthetic descriptor with enumerable and initializer properties (the later being non-standard.)
It seems clear that until this gets changed in TypeScript, we can't get lazy-initialize to work, and a number of the other tests, when called from TypeScript, are going to fail.
Typescript weirdness (part two)
Inside the TypeScript emitted __decorate method, if the descriptor is not provided, it looks one up using Object.getOwnPropertyDescriptor(target, key). For method decorators, this is returning a descriptor with the right value, but the configurable, enumerable, and writable properties are all set to true, at least in some of this projects test cases. The resulting behavior breaks more tests (e.g. one in decorate.spec.js) because the test is checking the enumerable property and finding it unexpectedly set.
For method decorators, the TypeScript emitted code does lookup the method's descriptor (using Object.getOwnPropertyDescriptor
@BurtHarris forgive my ignorance: is this still an issue after #133?
It depends...,
yes lazy initialize is still broken, but
no the tests don't fail because I removed its use from the utils.ts file.
I understand now why lazy initialize is broken, the inclusion of a "initialize" member in the descriptor passed to a property decorator wasn't part of the stage 0 proposal, and thus wasn't implemented by TypeScript.
@BurtHarris I'm OK with deprecating and removing lazyInitialize tbh. I'm doubtful any sizable number of people use it and now that we're dropping the debounce, etc I don't think we need it internally.
Gosh, I was really hoping it would work.
@BurtHarris are you a big user of lazyInitialize?
@BurtHarris are you saying that initialize property is causing the typescript decorators to fail?
@jayphelps No, I'm not a big user of lazyInitialize, it just sounds like a cool use of decorators.
@dtweedle I'm saying that without the initialize property in a descriptor (which I don't think was part of the stage 0 proposal) it is impossible to make lazyInitialize work. However with PR #133 (my typescriptify branch) all the build and test breaks are gone, you may want to have a look at that branch and try it out.
Jay may know if it's part of the stage-3 draft includes the initialize property, I'll have to read that document all over again, I've learned a lot about decorators since the last time I looked it over.
@BurtHarris it's still up in the air, but I find it unlikely both decorators and class fields will ship without interop between them. TC39 is still attempting to solidify these behaviors, the latest is here: https://github.com/littledan/proposal-unified-class-features/blob/master/DETAILS.md#member-descriptors where indeed it will be given an initializer it can wrap; though how decorators work are pretty different ever since stage-1 (everyone implements stage-0 as of now) so it's super unclear whether TypeScript will actually bite the bullet and ship that major breaking change eventually.