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

[RFC] feature(transformer): Add function overload support #303

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

martinjlowm
Copy link
Contributor

@martinjlowm martinjlowm commented Apr 11, 2020

edit: After a recent (write-up) discussion (see https://www.notion.so/RFC-Overload-c5a832ca51654f58a5019bf5507013a7), this PR is on hold. It'll add too much complexity in its current state and the project is not ready for such big changes. Feel free to leave a comment here if you have ideas or proposals for how we can approach this.


edit: Moved this PR into draft. It's dependent on a solution to the issue discussed in #313

edit: This now depends on #313 and #314

As the title states, this PR adds support for declared function overloads for (union) non Object-inherited types through the new option, transformOverloads, which is turned off by default. Turning this functionality off, enforces the transformer to select the first signature which is the same behavior as previously.
The runtime branching follows that of the TypeScript type checker, meaning the first overload has precedence over those that follow.

The signature of GetMethodDescriptor has been changed to accept an array of signatures that specifies parameter declarations and a return value.

Here's a snippet of how the transformation looks:

var functionMock = ɵExtension.Provider.instance.getMethod("exportedDeclaredOverloadedFunction", function (a, b, c) {
  if (typeof a === "boolean" && typeof b === "boolean" && typeof c === "number")
    return !1;
  else if (typeof a === "boolean" && typeof b === "boolean" && typeof c === "string")
    return !1;
  else if (typeof a === "boolean" && typeof b === "number" && typeof c === "number")
    return !1;
  else if (typeof a === "boolean" && typeof b === "number" && typeof c === "string")
    return !1;
  else if (typeof a === "boolean" && typeof b === "string" && typeof c === "number")
    return !1;
  else if (typeof a === "boolean" && typeof b === "string" && typeof c === "string")
    return !1;
  else if (typeof a === "number" && typeof b === "boolean" && typeof c === "boolean")
    return 0;
  else if (typeof a === "number" && typeof b === "boolean" && typeof c === "string")
    return 0;
  else if (typeof a === "number" && typeof b === "number" && typeof c === "boolean")
    return 0;
  else if (typeof a === "number" && typeof b === "number" && typeof c === "string")
  ...

This is applied to overloads in interfaces and of declared functions.

Currently, only non Object-inherited inputs are supported and the changes in this PR will fall back to instanceof Object if such arguments are provided with a warning that states so.

@uittorio
Copy link
Member

Hi :). I had a quick look! Really great work!

First of all I would like to review the strict null check then we can have a look at this PR.

I am also concerned about the impact on non supported values for overload methods.

I would divide the test in its own file as well.

@martinjlowm martinjlowm force-pushed the enhancement/support-overloads branch from 00902c1 to b3a4613 Compare April 14, 2020 16:51
@martinjlowm martinjlowm force-pushed the enhancement/support-overloads branch 7 times, most recently from 38a6b79 to 236184f Compare May 9, 2020 14:02
@martinjlowm martinjlowm changed the title enhancement(transformer): Add declared function overload support enhancement(transformer): Add function overload support May 16, 2020
@martinjlowm martinjlowm changed the title enhancement(transformer): Add function overload support feature(transformer): Add function overload support May 16, 2020
@uittorio
Copy link
Member

Hi, there are few things we could do to improve the readability of this branch

  1. I wouldn't include other branches not yet reviewed and merged. It makes more difficult for me to read it.

  2. Configuration features.
    Thanks for adding a new configuration.

I was thinking that we may want to add a features sections where you can pass strings for example

interface TsAutoMockOptions {
  debug: boolean;
  cacheBetweenTests: boolean;
  features: string[];
}

This will give as the advantage to add more features in the future in the same block.

  1. Documenting the features section and why we are doing it.
    This should be part of the ui sub-project. We need to create a new section in the options sections where we explain why we are adding a feature block.

  2. Testing testing testing
    It's okay to include this new feature in our transformer test but we should also test when it's not provided.
    I would like create two new test folder called featuresDisabled and featuresEnabled where we test just an happy case each

What do you think?

@martinjlowm martinjlowm marked this pull request as draft May 17, 2020 19:41
@martinjlowm
Copy link
Contributor Author

  1. I wouldn't include other branches not yet reviewed and merged. It makes more difficult for me to read it.

I'm with you on that one, but I had to include things from the other repos to get tests to pass :) One neat thing you can do though, is to pick a different range to diff in the "Changed files" tab, excluding the commits from the depending branch (PR). It's not an ideal solution, but it does help a little.

It was perhaps a bit early to review this, sorry - I haven't gotten around to push a refactor of the branching logic, it does clean things up a bit.

  1. Configuration features.
    Thanks for adding a new configuration.

I was thinking that we may want to add a features sections where you can pass strings for example

interface TsAutoMockOptions {
  debug: boolean;
  cacheBetweenTests: boolean;
  features: string[];
}

This will give as the advantage to add more features in the future in the same block.

Sounds like a good idea. However, I'd prefer to have the list of features be typed as well if so. But, ...

  1. Documenting the features section and why we are doing it.
    This should be part of the ui sub-project. We need to create a new section in the options sections where we explain why we are adding a feature block.

I'm not exactly sure why you want this plug-n-play feature. I'd expect to have everything enabled in a TDD-environment and not worry so much about performance, since things will be implemented as you go and the overhead of performance decreases as you progress. Maybe that's just me.

  1. Testing testing testing
    It's okay to include this new feature in our transformer test but we should also test when it's not provided.
    I would like create two new test folder called featuresDisabled and featuresEnabled where we test just an happy case each

I was actually thinking of this myself and I certainly think we should cover both scenarios. If the feature-gate is the way we'll approach this I suggest we test all permutations of features. And we may have to adjust the current test setup/CI accordingly.

@martinjlowm martinjlowm force-pushed the enhancement/support-overloads branch from 236184f to a06ff79 Compare May 19, 2020 20:24
@martinjlowm
Copy link
Contributor Author

martinjlowm commented May 19, 2020

I will take a look at documentation tomorrow :)

@martinjlowm martinjlowm force-pushed the enhancement/support-overloads branch from a06ff79 to 6eefd02 Compare May 19, 2020 20:39
@martinjlowm martinjlowm changed the title feature(transformer): Add function overload support [RFC] feature(transformer): Add function overload support May 28, 2020
@martinjlowm
Copy link
Contributor Author

martinjlowm commented Jun 11, 2020

I've come up with a completely different approach to this, see: martinjlowm@340b562

It works much better and is far simpler and it doesn't have all the problems than the branching logic implementation has per argument. This new approach scales linearly with the number of overload signatures, rather than the exponential scaling of this PR (num. signature times num. inputs)

cc: @uittorio

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.

2 participants