Lodash: flow (aka pipe): reverse order of overloads to fix various issues

[OliverJAsh]

Fixes #34379
Fixes #34380

This is the 2nd attempt. 1st attempt here: #33450. (It couldn't be merged due to memory issues when linting, and I'm hoping this is magically fixed now.)

To do

• Update wrappers
• Update flowRight

[OliverJAsh]

How can I add a test for the generic function issue (described in this PR's description)? Generics and pipe only work in 3.4.1, but these tests are ran against older versions.

declare const identity: <T>(t: T) => T;

// Expected and actual type: `<T>(a1: T) => T`
const fn1 = pipe(
    identity,
    identity,
);

// Expected type: `<T>(a1: T) => T`
// Actual type (prior to this PR): `(a1: {}) => {}`
const fn2 = pipe(
    identity,
    value => identity(value),
);

[OliverJAsh]

When these tests are bumped to TS version 3.4.1, we can do this instead:

        flow<A extends any[], B>(ab: (...args: A) => B): (...args: A) => B;
        flow<A extends any[], B, C>(ab: (...args: A) => B, bc: (b: B) => C): (...args: A) => C;
        flow<A extends any[], B, C, D>(ab: (...args: A) => B, bc: (b: B) => C, cd: (c: C) => D): (...args: A) => D;
        flow<A extends any[], B, C, D, E>(ab: (...args: A) => B, bc: (b: B) => C, cd: (c: C) => D, de: (d: D) => E): (...args: A) => E;
        flow<A extends any[], B, C, D, E, F>(ab: (...args: A) => B, bc: (b: B) => C, cd: (c: C) => D, de: (d: D) => E, ef: (e: E) => F): (...args: A) => F;
        flow<A extends any[], B, C, D, E, F, G>(ab: (...args: A) => B, bc: (b: B) => C, cd: (c: C) => D, de: (d: D) => E, ef: (e: E) => F, fg: (f: F) => G): (...args: A) => G;
        flow<A extends any[], B, C, D, E, F, G, H>(ab: (...args: A) => B, bc: (b: B) => C, cd: (c: C) => D, de: (d: D) => E, ef: (e: E) => F, fg: (f: F) => G, gh: (g: G) => H): (...args: A) => H;

microsoft/TypeScript#29904 (comment)

[OliverJAsh]

Hi @sandersn, this PR is failing the build due to memory problems when running the tests for Lodash. However, it works fine locally (#33450 (comment)). Can you provide any tips for how I can debug this? This is my 2nd attempt at this PR.

[typescript-bot]

@OliverJAsh Thank you for submitting this PR!

🔔 @bczengel @chrootsu @stepancar @aj-r @Ailrun @e-cloud @thorn0 @jtmthf @DomiR - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[typescript-bot]

@OliverJAsh The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

[sandersn]

@OliverJAsh The parallel runner leaks memory, I'm pretty sure. I should probably switch it to spawning subprocesses the way that the overnight run does. Since you can specify that the local run should have more memory, I think that should be as good as passing the CI checks.

Before checking in, though, I remember that lodash is already near the limit of causing memory problems in real-world usage. Can you check a couple of performance things before we check this in?

1. Try this change with a real, preferably large, project to make sure that it doesn't run out of memory there.
2. Compare the resource usage with-vs-without the change by passing --extendedDiagnostics to tsc.

[jgornick]

@OliverJAsh Would this fix utilize TypeScript 3.4 higher order type inference from generic functions, allowing composition of generic React higher-order components?

I've run into a case when using flow to compose react-redux.connect and react-i18next.translate, that only the return type from translate are being inferred, but not that from connect.

Am I incorrectly using flow here?

Example:

export const MyComponent = flow(
  connect(mapStateToProps, mapDispatchToProps),
  translate('common')
)(MyReactComponent)

Thank you!

[OliverJAsh]

@jgornick No this wouldn't fix that. See microsoft/TypeScript#30650

[OliverJAsh]

@jgornick Actually I was wrong, that issues only concerns generic components, which I'm guessing isn't your case.

[jgornick]

@OliverJAsh Well, if my component isn't a pure functional component, it won't work, right?

To expand on my previous example, MyReactComponent is defined as:

class MyReactComponent extends React.Component<MyReactProps, MyReactState> {
  // ...
}

[jgornick]

@OliverJAsh I'm also starting to think there my be something wrong with the types for react-redux.connect. If I just try to wrap MyReactComponent with connect, the resulting type is any.

[OliverJAsh]

@jgornick I would file a separate issue with steps to reproduce. Ping me and I'm happy to glance over it.

[OliverJAsh]

1. Try this change with a real, preferably large, project to make sure that it doesn't run out of memory there.

I'll give this a shot.

In any case this PR is on hold, because I realised that this change fixes the examples I provided, but breaks something else:

declare const pipe: {
    <A, B, C>(ab: (a: A) => B, bc: (b: B) => C): (a: A) => C;
    <A, B>(a: () => A, ab: (a: A) => B): () => B;
};

{
    // Expected type: `() => string`
    // Actual type: `(a: {}) => string`
    const fn1 = pipe(
        () => 'foo',
        x => x,
    );
}

I'm not sure how to create a happy medium here—something that preserves the behaviour above whilst also fixing the issues I mentioned (in this PR's description and in #33450). I asked the TS team here: microsoft/TypeScript#29904 (comment). The only solution I currently know of is generic rest parameters, but they require a newer TS version than what the Lodash typings currently support. If anyone has any ideas, I'd love to hear them.

[OliverJAsh]

The other option is we kill the 0-argument first function overloads. This would mean users have to pass in an empty object to the resulting function. Whilst this isn't ideal, it might be the only good option we have available:

const fn1 = pipe(
    () => 'foo',
    x => x,
);

fn1({})

[OliverJAsh]

I decided to go ahead and remove the 0-argument first function overloads, since it's the only way I know to fix #34379 and #34380.

This is a breaking change.

Usage before:

// () => string
const fn1 = pipe(
    () => 'foo',
    x => x,
);

fn1()

Usage after:

// (a1: {}) => string
const fn1 = pipe(
    () => 'foo',
    x => x,
);

// must pass empty object in here
fn1({})

Please could someone review this PR now? If we're happy with the changes, I will update wrappers and flowRight. /cc @bczengel @chrootsu @stepancar @aj-r @Ailrun @e-cloud @thorn0 @jtmthf @DomiR

[typescript-bot]

@OliverJAsh The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

[OliverJAsh]

This PR is failing the build due to memory problems when running the tests for Lodash. I need to debug this (#34374 (comment)), but before I try to do that, please can I get a review on the code changes? Once the theory is approved, I'll update the wrappers + flowRight and then I'll debug the build failure.

/cc code owners: @bczengel @chrootsu @stepancar @aj-r @Ailrun @e-cloud @thorn0 @jtmthf @DomiR

[typescript-bot]

@OliverJAsh I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

[typescript-bot]

@OliverJAsh To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

[aj-r]

Sorry for the late response - I don't think that removing the 0-argument overload is the right thing to do, though. I don't think it's worth it to break the 0-argument overload for everybody, in order to fix an edge case when strictFunctionTypes is false. I feel like the solution here is to change strictFunctionTypes to true rather than change the lodash types.

[OliverJAsh]

Thanks for the response @aj-r. This change isn't just concerning bugs related to strictFunctionTypes. In addition to #34379, see #34380.