ts-auto-mock
ts-auto-mock copied to clipboard
Typescript-TDD
feat(transformer): Support circular interface extensions
Utilize constructors to support circular generics for instantiable types.
If the transformer experiences circular generics, the scope descriptor parameter is now used to preserve a nested state to avoid looping descriptors forever. The scope enables the generic descriptor to determine whether to emit a new instance of the extension or to reuse the parent's constructor (which would emit an instance of the same prototype), i.e.:
getFactory("@factory")([{
i: ["@parameterId"],
w: function () { return new function () { Object.assign(this, ɵRepository.ɵRepository.instance.getFactory("@factory")([{
i: ["@parameterId"],
w: function () { return new this.constructor; }
}])); }; }
}])
┌────────────────────────┬────────┬──────────┬──────────┬─────────────┬──────────┬─────────┬─────────────┬──────────┬───────────┬────────────┬───────────┬────────────┬───────────┬────────────┐
│ (index) │ Files │ Lines │ Nodes │ Identifiers │ Symbols │ Types │ Memory used │ I/O read │ I/O write │ Parse time │ Bind time │ Check time │ Emit time │ Total time │
├────────────────────────┼────────┼──────────┼──────────┼─────────────┼──────────┼─────────┼─────────────┼──────────┼───────────┼────────────┼───────────┼────────────┼───────────┼────────────┤
│ no transformer │ '5093' │ '112415' │ '409280' │ '127866' │ '103042' │ '37318' │ '455358K' │ '0.35' │ '0.87' │ '1.45' │ '0.50' │ '2.94' │ '2.82' │ '7.71' │
│ no createMock │ '5093' │ '112415' │ '409280' │ '127866' │ '103042' │ '37318' │ '486916K' │ '0.30' │ '0.86' │ '1.37' │ '0.50' │ '2.89' │ '2.98' │ '7.75' │
│ One interface per file │ '5098' │ '112431' │ '439436' │ '137914' │ '103077' │ '32346' │ '508633K' │ '0.33' │ '0.84' │ '1.52' │ '0.50' │ '3.12' │ '3.53' │ '8.67' │
│ Interface reuse │ '5098' │ '97431' │ '444436' │ '132914' │ '93078' │ '27346' │ '499714K' │ '0.32' │ '0.95' │ '1.47' │ '0.55' │ '3.20' │ '3.67' │ '8.90' │
│ One Interface / Reuse │ '5098' │ '104931' │ '441936' │ '135414' │ '98078' │ '29846' │ '504858K' │ '0.30' │ '0.90' │ '1.49' │ '0.50' │ '3.29' │ '3.71' │ '8.99' │
└────────────────────────┴────────┴──────────┴──────────┴─────────────┴──────────┴─────────┴─────────────┴──────────┴───────────┴────────────┴───────────┴────────────┴───────────┴────────────┘
I just went ahead and opened this here (cc: @uittorio) - it's easier than having a commit hash lying around in some random conversation.
Hi @martinjlowm. @Pmyl and I had a look at your changes. This is great work.
Pmyl found a scenario where this implementation will not work. Could you add a test ?
interface B<T> {
prop: T;
}
interface A extends B<A> {
a: boolean;
test: A;
}
const type: A = createMock<A>();
expect(type.test.prop.a).toBe(false); // undefined
the reason this is not working its because generic are cached and re used. I think in your implementation you have to make sure the value w will be cached correctly. I can have a look later but I'm sure you'll find the solution :P
w: function () {
return new this.constructor;
}
Hi @martinjlowm. @Pmyl and I had a look at your changes. This is great work.
Pmyl found a scenario where this implementation will not work. Could you add a test ?
interface B<T> { prop: T; } interface A extends B<A> { a: boolean; test: A; } const type: A = createMock<A>(); expect(type.test.prop.a).toBe(false); // undefinedthe reason this is not working its because generic are cached and re used. I think in your implementation you have to make sure the value w will be cached correctly. I can have a look later but I'm sure you'll find the solution :P
w: function () { return new this.constructor; }
I see!
So, the fix is easy, but I'd like some thoughts on some naming.
The problem lies here: https://github.com/Typescript-TDD/ts-auto-mock/blob/57727e6de0219146a621f0a9ee376088c8f9e211/src/transformer/genericDeclaration/genericDeclaration.ts#L160-L162
The mock key for the new scope may conflict with other parallel scopes and the fix is as simple as:
const constructedDeclarationKey = `${declarationKey}_C`;
if (!typeParameterDeclaration || scope.currentMockKey !== constructedDeclarationKey) {
genericValueDescriptor = GetDescriptor(genericNode, new Scope(constructedDeclarationKey));
}
Which will uniquely identify the scope with the appended _C (c, for constructed and _ to keep it consistent with generated keys) and the constructor will always be emitted on the first occurrence.
Alternatively, we may also extend Scope with a "bind"-state and toggle it on here (to know that a constructor has been emitted, thus binding this). I don't know, maybe, it'll become useful in other scenarios.
if (!typeParameterDeclaration || !scope.isBound()) {
genericValueDescriptor = GetDescriptor(genericNode, (new Scope(declarationKey)).bind());
}
What do You think?
I've found another problem with a much simpler example.
It seems like the this.constructor code doesn't work as expected, in your tests you're not hitting that code so they don't fail.
export class ClassWithGenerics<T> {
public a: T;
}
interface A extends ClassWithGenerics<A> {
b: number;
}
createMock<A>();
This is the generated code:
ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@ClassWithGenerics_1T"],
w: function () {
return new function () {
Object.assign(this, ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@ClassWithGenerics_1T"],
w: function () {
return new this.constructor;
}
}]));
};
}
}]);
This is your test: https://github.com/Typescript-TDD/ts-auto-mock/pull/389/commits/99cb2de0631740f710f9df502f41aefe78e5be3b#diff-a93f7cd70330ab19d1c4df5c1b3bd0e7R258
expect(propertiesA.a.b).toBe(0);
That hits only the code with Object.assign, to hit the return new this.constructor we need to go deeper and use again the a property:
expect(propertiesA.a.a.b).toBe(0);
That will fail because b is undefined. It seems like the last a is an empty object
Right, that makes sense - I didn't realize class properties were emitted differently to interface properties. this is not bound correctly for class properties. I'll investigate :)
I’m looking forward the new solution, I was positively surprised by how smart this solution is, too bad it doesn’t work for every case
It appears there was a couple of issues:
- The cached declaration key issue, I fixed by implementing the proposed
Scopechange. And actually, this led to the following issue. -
Object.assigninvokes the getter's when copying, leading to an inifinite recursion in runtime. This is fixed by using a combination ofObject.definePropertiesandObject.getOwnPropertyDescriptors. That also implies, that if you were to log the reference to stdout, thetoStringfunction will result in the same infinite recursion. -
thisis now temporarily stored asthatfrom theMockIdentifierGenericCircularReferenceidentifier to preserve the reference.
I went ahead and bumped the test to cover the actual nesting. I didn't realize that I had previously tested by referencing the non-generic properties which is completely irrelevant to this PR 🤦
I have few comments on it but before leaving them I'm spending a bit of time understanding the reason behind every change. At the moment the behaviour seems fine but I'll let you know when I read and ran everything locally, sorry for the delay
I had some time to test some edge cases and I noticed something interesting in the generated code:
In case of a double circular dependency
interface GenC<T> {
c: T;
}
interface GenB<T> extends GenC<GenB<T>> {
b: T;
}
interface A extends GenB<A> {
a: A;
value: string;
}
there is a piece of generated code that goes like this
[...]
Object.defineProperties(m, {
"a": {
get: function () {
return d.hasOwnProperty("a") ? d["a"] : d["a"] = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenB_1T"],
w: function () {
return new function () {
var that = this;
Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenB_1T"],
w: function () {
return new that.constructor;
}
}, {
i: ["@GenC_1T"],
w: function () {
return new that.constructor;
}
}])));
};
}
}, {
[...]
In these generics @GenB_1T makes sense to be "new that.constructor" because that generic has to generate another instance of @A_1.
The problem is in @GenC_1T, that shouldn't be A but GenB<A>.
I don't think this can cause particular problems because A extends GenB<A> so for the rules of polymorphism those are actually matching!
But in general I would like to make sure that these (currently failing) tests pass correctly
it('should work', () => {
interface GenC<T> {
c: T;
}
interface GenB<T> extends GenC<GenB<T>> {
b: T;
}
interface A extends GenB<A> {
a: A;
value: string;
}
const type: A = createMock<A>();
expect((type.a.c.b as unknown as A).value).not.toEqual('');
expect((type.a.b as unknown as A).value).not.toEqual('');
expect((type.a.b.c as unknown as A).value).not.toEqual('');
expect((type.b.c as unknown as A).value).not.toEqual('');
expect((type.b as unknown as A).value).not.toEqual('');
});
As I said this can be a non-issue if nobody casts anything to unknown/any but I didn't spent much time to think of worse outcomes. If someone want to ignore this small issue please let me know, we can take a decision
I just found a more problematic case:
interface GenC<T> {
c: T;
}
interface GenB<T> {
b: T;
}
interface B {
valueB: string;
}
interface A extends GenB<A>, GenC<B> {
a: A;
valueA: string;
}
const type: A = createMock<A>();
expect((type.a.b.c as unknown as A).valueA).not.toEqual(''); //fail
expect(type.a.b.c.valueB).toEqual(''); //fail
expect((type.b.c as unknown as A).valueA).not.toEqual(''); //fail
expect(type.b.c.valueB).toEqual(''); //fail
Generated code:
var type = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenB_1T"],
w: function () {
return new function () {
var that = this;
Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenB_1T"],
w: function () {
return new that.constructor;
}
}, {
i: ["@GenC_1T"],
w: function () {
return new that.constructor;
}
}])));
};
}
}, {
i: ["@GenC_1T"],
w: function () {
return new function () {
var that = this;
Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@B_1")([])));
};
}
}]);
As you can see the generic @GenC_1T is going to be A (new that.constructor) but it should be B. This breaks cases where no cast to any/unknown happened.
I'm not sure why this happens really, I think it's something about the scope... unfortunately this feature is more complex than expected
I see! Nice that you found some complex test cases for this. In general, in both examples, it seems we need to apply some extra logic to this "bind"-scope behavior - a simple scope check is not enough since the generic variable may change in that same iteration, in the case where there are more than a single extension applied.
I hope to find some time over the weekend to test against those two examples and find a solution. I will keep you posted :)
Just an update - I had a look at these extra cases yesterday and I'm close to a solution - what's left, is being able to distinguish the that assignment and a reference to it, since reassignments may break previous references (through nesting). I updated the "bind"-scope change to bind for generic keys to avoid the conflicts when multiple extensions are present, i.e. a Set for bind() -> bindFor(key: string).
454713fb2261d14c4dcb4fa5b06b87c7dcbb0117 solves the issue you discovered @Pmyl - I rewrote the extends test to cover those cases and a range of parallel circular extensions. MockIdentifierGenericCircularReference is replaced with a variable named as the uniqueName in createGenericParameter, with @ replaced by an _. We may have to name it differently to avoid variable clashing - perhaps by using MockPrivatePrefix instead? What do You guys think?
edit: That was perhaps a bit too quick I commented - discovered some cases that weren't covered.
A quick update.
With the two most recent commits, the emitted code for a fairly complex extension configuration is as follows:
ɵRepository.ɵRepository.instance.registerFactory("@B_1", function (t) { return (function () { var d = {}, m = { "b": "" }; Object.defineProperties(m, {
"A": { get: function () { return d.hasOwnProperty("A") ? d["A"] : d["A"] = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenericC_1T"],
w: function () { return new function () { var ɵA_1_GenericC_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenericC_1T"],
w: function () { return new ɵA_1_GenericC_1T.constructor; }
}, {
i: ["@GenericD_1T"],
w: function () { return new function () { var ɵA_1_GenericD_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenericC_1T"],
w: function () { return new ɵA_1_GenericC_1T.constructor; }
}, {
i: ["@GenericD_1T"],
w: function () { return new ɵA_1_GenericD_1T.constructor; }
}, {
i: ["@GenericE_1T"],
w: function () { return new function () { var ɵA_1_GenericE_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@B_1")([{
i: ["@GenericC_1T"],
w: function () { return new function () { var ɵB_1_GenericC_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@B_1")([{
i: ["@GenericC_1T"],
w: function () { return new ɵB_1_GenericC_1T.constructor; }
}, {
i: ["@GenericD_1T"],
w: function () { return new function () { var ɵB_1_GenericD_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenericC_1T"],
w: function () { return new ɵA_1_GenericC_1T.constructor; }
}, {
i: ["@GenericD_1T"],
w: function () { return new ɵA_1_GenericD_1T.constructor; }
}, {
i: ["@GenericE_1T"],
w: function () { return new ɵA_1_GenericE_1T.constructor; }
}]))); }; }
}, {
i: ["@GenericE_1T"],
w: function () { return new function () { var ɵB_1_GenericE_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
i: ["@GenericC_1T"],
w: function () { return new ɵA_1_GenericC_1T.constructor; }
}, {
i: ["@GenericD_1T"],
w: function () { return new ɵA_1_GenericD_1T.constructor; }
}, {
i: ["@GenericE_1T"],
w: function () { return new ɵA_1_GenericE_1T.constructor; }
}]))); }; }
}]))); }; }
}, {
...
The naming of A_1_GenericC_1T relates to the GenericC<T> implementation of A.
Since these constructor references are only valid for the current scope, this quickly builds up a lot of extra code, that I think can be reused to some degree. At least the scope functionality is there, so registration of cached functions should be fairly straightforward to implement.
Currently, There's still at least one uncovered case I know of:
interface A extends GenericC<A>, GenericD<A>, GenericE<B>, GenericFG<A, B> {
a: number;
B: B;
}
Here, GenericFG does not bind A and B properly. The bind functionality, does not consider more than one type argument. I hope the fix for that is easy :)