Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(transformer): Ensure mocked interfaces don't extend themselves infinitely if passed as generic argument #312

Merged

Conversation

martinjlowm
Copy link
Contributor

@martinjlowm martinjlowm commented May 8, 2020

As the title states, type arguments weren't considered in cases where the interface itself was passed as a generic argument, which would result in a circular extension. If they are indeed passed as a generic argument to the extending class or interface they will already have been defined (/mocked). These changes terminates early to make sure the logic doesn't continue forever.

This silences #183 and warns about the limitation instead.

@uittorio
Copy link
Member

uittorio commented May 8, 2020

Hi :). Thanks for starting a discussion about this annoying error. The solution you are applying to the generic declaration functionality will definitely work for that specific scenario but it may break for existing functionality that we support. I'll try to type here a code example of what it could break.

class A {
   propA: string
}

class C<T> {
 propC: T
}

class B extends C<A> {
   
}
const a = createMock<A>();
const b = createMock<B>();

expect(b.propC.propA).toBe("");

If I am not mistaken propA will be undefined because the generic A will be already present in the declarationKey because already mocked

I think we should add a test first to generic already been mocked by ts-auto-mock

FYI
I have a WIP branch for this functionality. Hopefully, I'll get the time to finish it. If we really want to just disable this scenario you will have to detect that A is the same declaration of the created mock and if it's present the keyDeclarationMap.

@martinjlowm
Copy link
Contributor Author

martinjlowm commented May 9, 2020

I threw your example into the playground file and got the following output:

it('should work', function () {
  var A = /** @class */ (function () {
    function A() {
    }
    return A;
  }());
  var C = /** @class */ (function () {
    function C() {
    }
    return C;
  }());
  var B = /** @class */ (function (_super) {
    __extends(B, _super);
    function B() {
      return _super !== null && _super.apply(this, arguments) || this;
    }
    return B;
  }(C));
  var a = ɵRepository.ɵRepository.instance.getFactory("@A_1")([]);
  var b = ɵRepository.ɵRepository.instance.getFactory("@B_1")([{
    i: ["@C_1T"],
    w: function () { return ɵRepository.ɵRepository.instance.getFactory("@A_1")([]); }
  }]);
  expect(b.propC.propA).toBe('');
});

With these changes, propA is indeed a string and the test passes.

But let me know if you have any tests in your WIP-branch that will make this fail.

@uittorio
Copy link
Member

uittorio commented May 9, 2020

Hi :) You are right! I didn't read the code correctly at first. This would be a first good approach to just avoiding an infinite loop. However, we should document this unsupported behavior.
I would do a couple of things to keep track of this:

  • Can you add an entry in Unsupported types?
  • Add a comment in that new line of code that links to the unsupported type page so we can find it easily when we want to apply a better solution
class C<T> {
    propC: T
    test: string
  }

  class A extends C<A> {
    propA: number
  }
  const b = createMock<A>();

  expect(b.propC.propC.test).toBe(""); // this will fail because we will not support generics of the same type

Basically we would avoid checking any generic types that its the same of the type class that you are trying to mock.

@martinjlowm
Copy link
Contributor Author

martinjlowm commented May 9, 2020

I suppose the proper way of implementing this, is to store the value of w elsewhere and give it a proper name so it can reference itself, e.g. something along the lines, from:

var a = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
    i: ["@C_1T"],
    w: function () { return ɵRepository.ɵRepository.instance.getFactory("@A_1")([]); }
  }]);

to:

ɵRepository.ɵRepository.instance.registerGenericValue("@G_1", function () {
  return ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
    i: ["@C_1T"],
    w: ɵRepository.ɵRepository.instance.getGenericValue("@G_1")(),
  }])
});
var a = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
  i: ["@C_1T"],
  w: ɵRepository.ɵRepository.instance.getGenericValue("@G_1")(),
}]);

How are you currently approaching it?

@uittorio
Copy link
Member

uittorio commented May 9, 2020

Thats precisesly how I am currently approaching this :) of course there are stell few things to finish

@martinjlowm
Copy link
Contributor Author

I'll update the types-not-supported.mdx and throw in a warning (and a comment) in addition to the current changes:

WARNING: Transformer - Found a circular generic of `A' and such generics are currently not supported. The generated mock will be incomplete.

Stay tuned :p

@martinjlowm
Copy link
Contributor Author

By the way, I will gladly help out if you have your WIP branch lying around somewhere to fix this for good.

@uittorio
Copy link
Member

uittorio commented May 9, 2020

Thats great!!! Thanks :) i should be able to openthe branch later or tomorrow morning :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants