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 lodash.* packages - remove re-export `compose` #1478

Merged
merged 11 commits into from Dec 27, 2017

Conversation

Projects
None yet
4 participants
@rosskevin
Copy link
Collaborator

rosskevin commented Dec 27, 2017

Closes #1445. Supercedes #1474

Updated summary

We were re-exporting flowright a.k.a. compose which was implicitly or explicitly using the any type - which is not a good idea. Instead, remove this re-export and allow users to choose their composition method on their own.

Migration

User may migrate by:

  1. yarn add lodash @types/lodash && using with const compose = require('lodash/flowRight); or
  2. yarn add recompose @types/recompose && using with const compose = require('recompose/compose')

Other changes

  • lock down bundle max size to 4.7 KB prior to change (4.67KB actual)
  • remove lodash.pick and lodash.flowright packages
  • use lodash/pick which has types in @types/lodash
  • re-compile and check file size
  • fix inconsistency with flow types QueryProps -> GraphqlQueryControls

@rosskevin rosskevin requested a review from excitement-engineer Dec 27, 2017

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 27, 2017

I see in the compiled lib/index.d.ts that compose is exported as any. So while our internal usage is fully typed, we are re-exporting something as an explicit any. While some might be good with it, it seems like we are dumbing down types.

lib/index.d.ts snippet:

declare const compose: any;
export { compose };

It seems like we would be better off excluding this from our exports and recommending users add:

  • lodash dependency
  • const compose = require('lodash/flowright')
  • or anything else they choose such as recompose/compose and @types/recompose

So they can get explicit typing. I don't want to copy in the lodash types because it is unwieldy:

    //_.flowRight
    interface LoDashStatic {
        /**
         * This method is like _.flow except that it creates a function that invokes the provided functions from right
         * to left.
         *
         * @param funcs Functions to invoke.
         * @return Returns the new function.
         */
        // 0-argument first function
        flowRight<R2, R1>(f2: (a: R1) => R2, f1: () => R1): () => R2;
        flowRight<R3, R2, R1>(f3: (a: R2) => R3, f2: (a: R1) => R2, f1: () => R1): () => R3;
        flowRight<R4, R3, R2, R1>(f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: () => R1): () => R4;
        flowRight<R5, R4, R3, R2, R1>(f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: () => R1): () => R5;
        flowRight<R6, R5, R4, R3, R2, R1>(f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: () => R1): () => R6;
        flowRight<R7, R6, R5, R4, R3, R2, R1>(f7: (a: R6) => R7, f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: () => R1): () => R7;
        // 1-argument first function
        flowRight<A1, R2, R1>(f2: (a: R1) => R2, f1: (a1: A1) => R1): (a1: A1) => R2;
        flowRight<A1, R3, R2, R1>(f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1) => R1): (a1: A1) => R3;
        flowRight<A1, R4, R3, R2, R1>(f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1) => R1): (a1: A1) => R4;
        flowRight<A1, R5, R4, R3, R2, R1>(f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1) => R1): (a1: A1) => R5;
        flowRight<A1, R6, R5, R4, R3, R2, R1>(f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1) => R1): (a1: A1) => R6;
        flowRight<A1, R7, R6, R5, R4, R3, R2, R1>(f7: (a: R6) => R7, f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1) => R1): (a1: A1) => R7;
        // 2-argument first function
        flowRight<A1, A2, R2, R1>(f2: (a: R1) => R2, f1: (a1: A1, a2: A2) => R1): (a1: A1, a2: A2) => R2;
        flowRight<A1, A2, R3, R2, R1>(f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2) => R1): (a1: A1, a2: A2) => R3;
        flowRight<A1, A2, R4, R3, R2, R1>(f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2) => R1): (a1: A1, a2: A2) => R4;
        flowRight<A1, A2, R5, R4, R3, R2, R1>(f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2) => R1): (a1: A1, a2: A2) => R5;
        flowRight<A1, A2, R6, R5, R4, R3, R2, R1>(f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2) => R1): (a1: A1, a2: A2) => R6;
        flowRight<A1, A2, R7, R6, R5, R4, R3, R2, R1>(f7: (a: R6) => R7, f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2) => R1): (a1: A1, a2: A2) => R7;
        // 3-argument first function
        flowRight<A1, A2, A3, R2, R1>(f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3) => R1): (a1: A1, a2: A2, a3: A3) => R2;
        flowRight<A1, A2, A3, R3, R2, R1>(f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3) => R1): (a1: A1, a2: A2, a3: A3) => R3;
        flowRight<A1, A2, A3, R4, R3, R2, R1>(f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3) => R1): (a1: A1, a2: A2, a3: A3) => R4;
        flowRight<A1, A2, A3, R5, R4, R3, R2, R1>(f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3) => R1): (a1: A1, a2: A2, a3: A3) => R5;
        flowRight<A1, A2, A3, R6, R5, R4, R3, R2, R1>(f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3) => R1): (a1: A1, a2: A2, a3: A3) => R6;
        flowRight<A1, A2, A3, R7, R6, R5, R4, R3, R2, R1>(f7: (a: R6) => R7, f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3) => R1): (a1: A1, a2: A2, a3: A3) => R7;
        // 4-argument first function
        flowRight<A1, A2, A3, A4, R2, R1>(f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3, a4: A4) => R1): (a1: A1, a2: A2, a3: A3, a4: A4) => R2;
        flowRight<A1, A2, A3, A4, R3, R2, R1>(f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3, a4: A4) => R1): (a1: A1, a2: A2, a3: A3, a4: A4) => R3;
        flowRight<A1, A2, A3, A4, R4, R3, R2, R1>(f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3, a4: A4) => R1): (a1: A1, a2: A2, a3: A3, a4: A4) => R4;
        flowRight<A1, A2, A3, A4, R5, R4, R3, R2, R1>(f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3, a4: A4) => R1): (a1: A1, a2: A2, a3: A3, a4: A4) => R5;
        flowRight<A1, A2, A3, A4, R6, R5, R4, R3, R2, R1>(f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3, a4: A4) => R1): (a1: A1, a2: A2, a3: A3, a4: A4) => R6;
        flowRight<A1, A2, A3, A4, R7, R6, R5, R4, R3, R2, R1>(f7: (a: R6) => R7, f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (a1: A1, a2: A2, a3: A3, a4: A4) => R1): (a1: A1, a2: A2, a3: A3, a4: A4) => R7;
        // any-argument first function
        flowRight<R2, R1>(f2: (a: R1) => R2, f1: (...args: any[]) => R1): (...args: any[]) => R2;
        flowRight<R3, R2, R1>(f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (...args: any[]) => R1): (...args: any[]) => R3;
        flowRight<R4, R3, R2, R1>(f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (...args: any[]) => R1): (...args: any[]) => R4;
        flowRight<R5, R4, R3, R2, R1>(f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (...args: any[]) => R1): (...args: any[]) => R5;
        flowRight<R6, R5, R4, R3, R2, R1>(f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (...args: any[]) => R1): (...args: any[]) => R6;
        flowRight<R7, R6, R5, R4, R3, R2, R1>(f7: (a: R6) => R7, f6: (a: R5) => R6, f5: (a: R4) => R5, f4: (a: R3) => R4, f3: (a: R2) => R3, f2: (a: R1) => R2, f1: (...args: any[]) => R1): (...args: any[]) => R7;
        flowRight(f7: (a: any) => any, f6: (a: any) => any, f5: (a: any) => any, f4: (a: any) => any, f3: (a: any) => any, f2: (a: any) => any, f1: () => any, ...funcs: Array<Many<(...args: any[]) => any>>): (...args: any[]) => any;
        flowRight(funcs: Array<Many<(...args: any[]) => any>>): (...args: any[]) => any;
    }

Is there any reason we need to be re-exporting/dumbing down something as compose when the user can make their own choices (e.g. they may want to use recompose/compose instead)?

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 27, 2017

@jbaxleyiii @excitement-engineer I propose we remove the re-export of flowright as compose because it is dumbing down the type-checking, and we are assuming it is how users will want to compose.

@rosskevin rosskevin requested a review from jbaxleyiii Dec 27, 2017

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator

excitement-engineer commented Dec 27, 2017

I agree. I think the docs can point out some the various alternatives and let the user decide which to use.

rosskevin added some commits Dec 27, 2017

@rosskevin rosskevin changed the title Fix lodash.* packages to use lodash/* packages - typed with @types/lodash Fix lodash.* packages - remove re-export `compose` Dec 27, 2017

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator

excitement-engineer commented Dec 27, 2017

Can you add the breaking change in the flow type name to the changelog as well? Otherwise it looks good

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 27, 2017

Already in the changelog but I'll add the second link to this PR.

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 27, 2017

@excitement-engineer tests are failing on travis only - something with imports. I tweaked the jest config a bit but I'm at a loss. Wiped out node_modules locally and still all good. Wiped out cache on travis and it still fails. Makes no sense.

 FAIL  test/client/graphql/shared-operations.test.tsx
  ● Test suite failed to run
    Cannot find module 'lodash/flowright' from 'shared-operations.test.tsx'
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:191:17)
      at Object.<anonymous> (test/client/graphql/shared-operations.test.tsx:18:17)

@rosskevin rosskevin merged commit 08cb715 into apollographql:master Dec 27, 2017

4 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./lib/umd/react-apollo.js: 4.65KB < maxSize 4.7KB gzip (27B smaller than master, good job!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 96.388%
Details

@rosskevin rosskevin deleted the rosskevin:flowright branch Dec 27, 2017

rosskevin added a commit to rosskevin/react-apollo that referenced this pull request Dec 27, 2017

Fix lodash.* packages - remove re-export `compose` (apollographql#1478)
* first lock down bundle size

* Remove lodash.* packages and use lodash/* packages - identical bundle size

* Remove re-export of flowright due to loss of types (exports as `any`)

* fix import/usage of compose

* fix inconsistency with flow types QueryProps -> GraphqlQueryControls

* fix some jest configurations - no idea why module resolution is failing on travis

* remove unused lodash-es from jest transformIgnore

* add ref to PR in changelog

* change capitalization on flowRight
@develomark

This comment has been minimized.

Copy link

develomark commented Jan 3, 2018

+1 on this approach, however, I simply replaced:

import * as compose from 'lodash.flowright';
export { compose };

with

import { flowRight as compose} from "lodash";
export { compose };

in browser.d.ts to fix TS compile errors.

@ryanflowers

This comment has been minimized.

Copy link

ryanflowers commented Jan 11, 2018

Hello @rosskevin I see you fixed the flowright typing issue and merged it to master. Thank you! I'm just wondering when it will be published in npm for consumption..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment