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

Dependencies update, typescript check tests, fix hoc inference #1402

Merged
merged 75 commits into from Dec 18, 2017

Conversation

Projects
None yet
4 participants
@rosskevin
Copy link
Collaborator

rosskevin commented Dec 11, 2017

testing privately (all good so far), but feel free if you want to ping me to confirm before squash+merge

Summary

This PR is largely housekeeping and the fallout from dependency updates. There are no runtime changes included in this PR with the exception of removing the one deprecated skip prop.

  • Jest 20->21 required some test assertion changes for which I created a custom matcher.
  • Tests are all now valid typescript and pass tslint
  • Remove deprecated operationOptions.options.skip, use operationOptions.skip instead

Typescript summary

Typescript types have been refactored for consistency and a focus on typescript user experience. This means consistent parameterized type names, types have been refactored and exported individually, and changes made to the signature (exported type names were not changed).

  • Fix graphql hoc inference type signature
  • Make all test sources type check (closes #1404)
  • Greater reuse of types types.ts
  • Fix shouldResubscribe props on OperationOption
  • graphql parameterized types streamlined for a) full typing; and b) ease of use; and c) consistency. New parameterized usage is: graphql<TProps, TData, TGraphQLVariables, TChildProps> where none are required and full typing only requires the first three params (TChildProps can be derived). The first three params will be most typical when used in conjunction with apollo-codegen. Note the ordering of graphql parameters is different to be consistent with ChildProps and variants and to minimize verbosity (supersedes #1395)
  • Add ChildDataProps and ChildMutateProps for optional stronger typed usage version of ChildProps, and can be used as the fourth parameter to graphql. This is in preparation for a potential split query vs mutation usage of the graphql component.

Misc

  • Update all dependencies
  • Update script usage - remove any deprecations
  • Update jest setup
  • Update prettier setup
  • Update typescript setup
  • Run lint-fix (prettier + tslint --fix)
  • Remove integration tests/samples for external libraries (out of date and not necessary for unit testing)

rosskevin added some commits Dec 11, 2017

Fix TS4053 error with latest @types/react
Error:(45, 3) TS4053: Return type of public method from exported class has or is using name 'React.ReactElement' from external module "react-apollo/node_modules/@types/react/index" but cannot be named.
@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Dec 11, 2017

Fails
🚫

Danger failed to run dangerfile.ts.

Error SyntaxError

Unexpected token import
dangerfile.ts:1
(function (exports, require, module, __filename, __dirname) { import { includes } from 'lodash';
                                                              ^^^^^^

SyntaxError: Unexpected token import
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:599:28)
    at requireFromString (/home/travis/build/apollographql/react-apollo/node_modules/require-from-string/index.js:28:4)
    at Object.<anonymous> (/home/travis/build/apollographql/react-apollo/node_modules/danger/distribution/runner/runners/inline.js:96:21)
    at step (/home/travis/build/apollographql/react-apollo/node_modules/danger/distribution/runner/runners/inline.js:32:23)
    at Object.next (/home/travis/build/apollographql/react-apollo/node_modules/danger/distribution/runner/runners/inline.js:13:53)
    at /home/travis/build/apollographql/react-apollo/node_modules/danger/distribution/runner/runners/inline.js:7:71
    at new Promise (<anonymous>)
    at __awaiter (/home/travis/build/apollographql/react-apollo/node_modules/danger/distribution/runner/runners/inline.js:3:12)

Dangerfile

75|     fail('Please add a description to your PR.');
76|   }
77| 
78|   const hasAppChanges = modifiedAppFiles.length > 0;
79| 
------------^
80|   const hasTestChanges = modifiedTestFiles.length > 0;
81| 
82|   // Warn when there is a big PR
83|   const bigPRThreshold = 500;

Generated by 🚫 dangerJS

@rosskevin rosskevin changed the title Dependencies update WIP Dependencies update Dec 12, 2017

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 12, 2017

My last push is broken, it is failing with errors such as:

Expected value to equal:
  {"people": [{"name": "Luke Skywalker"}]}
Received:
  {"people": [{"name": "Luke Skywalker", Symbol(id): "$ROOT_QUERY.allPeople({\"first\":1}).people.0"}], Symbol(id): "$ROOT_QUERY.allPeople({\"first\":1})"}

I found this somewhat related SO post, except ironically we aren't trying to get the Symbol(id), it is just showing up. I moved to typescript recently from flow, so maybe this is just something common I haven't bumped into.

It could be a problem with matcher toEqual (unlikely), or the transformation of data to the prop could be errantly putting a literal Symbol(id) in (seems more likely). I can only surmise this is caused by a dependency update, but it seems that I incrementally had good-ish builds all the way up to the jest/danger update.

@jbaxleyiii any thoughts/guidance here? I'd like to get this PR to the finish line.

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 12, 2017

The upgraded jest isEqual is aware of symbols, so several tests are failing.

It seems expect(props.data.allPeople).toEqual(data1.allPeople) will need to be the equivalent of

expect(JSON.parse(JSON.stringify(props.data.allPeople))).toEqual(
  data1.allPeople,
);

I'm making a utility method to serialize/deserialize to wash out the symbols for comparison to the hardcoded fixtures.

rosskevin added some commits Dec 12, 2017

New jasmine custom matcher added - typescript still unhappy about it.
- Reorganized generic typings file into a folder with module per file layout
- Moved jest setup files under test/setup (with matcher)
Fix graphql decorator inference and tie together parameterized types …
…(breaking change)

Fixes issue #755, Supercedes PR #1395 - and making progress towards valid typescript in the test directory #1404
@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 14, 2017

I'm going to publish this privately and start integrating with our codebase to ensure no errors in typing that I didn't already uncover with the tests.

@orta

This comment has been minimized.

Copy link

orta commented Dec 14, 2017

@rosskevin - I'd recommend updating Danger to 2.x, it stops relying on Jest internals then 👍

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

PR apollographql#1402
commit eb0842b
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 11:10:55 2017 -0600

    rename custom matcher to easier toEqualJson

commit ecf05dc
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 10:34:28 2017 -0600

    update changelog

commit 5ebe7d0
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 09:51:35 2017 -0600

    change jest config to see if it impacts danger

commit 675002f
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 09:30:22 2017 -0600

    yarn test passes

commit 69a7ca6
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 09:16:05 2017 -0600

    update typescript usage

commit ff65cce
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 09:15:41 2017 -0600

    examples are not running in master by jest, but evidently the jest upgrade sees this config and needs more work or example module resolution

commit c40370a
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 08:59:37 2017 -0600

    consolidate test-utils

commit 5983157
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Thu Dec 14 08:47:38 2017 -0600

    tests are linted

commit 926d12e
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 19:29:11 2017 -0600

    fix compile script

commit a032e01
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 19:24:09 2017 -0600

    yarn type-check runs

commit e2b6355
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 19:24:00 2017 -0600

    flow really out of date - some updates to remove redux but not perfect

commit 65d5d4b
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 19:07:40 2017 -0600

    component.test type checks

commit 2a53d8f
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 18:59:09 2017 -0600

    remove more integration tests, server tests type check

commit 198005d
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 17:54:34 2017 -0600

    remove redux integration test

commit 9113a1e
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 17:47:03 2017 -0600

    server/index.test type checks

commit 227b439
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 17:46:48 2017 -0600

    ApolloProvider.test type checks

commit 57d7298
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 17:31:54 2017 -0600

    mobx test works, but redux form tests were out of date. typescript updates applied but needs more work

commit f13fe1a
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 15:58:51 2017 -0600

    subscriptions test type checks

commit d8d7af5
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 15:52:03 2017 -0600

    fix shouldResubscribe props

commit 53efa16
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:54:05 2017 -0600

    fix prettier text jumping and specify the default width just for clarity

commit 863718a
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:45:05 2017 -0600

    wip statics

commit 5d1b74d
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:32:54 2017 -0600

    add stateless function usage example

commit e8f096d
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:21:08 2017 -0600

    type checked shared-operations

commit b13bca2
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:20:49 2017 -0600

    TData and TGraphQLVariables naming consistency. Expose DataValue separately, Expose stronger typed ChildDataProps and ChildMutateProps

commit e7039dd
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:19:36 2017 -0600

    disallow synthetic defaults to raise errors sooner

commit 24c37ab
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:19:01 2017 -0600

    consistent naming of TData instead of TResult which populates the DataValue

commit ae12c3e
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 14:18:23 2017 -0600

    make purpose of file more clear

commit 3f8d7c8
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Wed Dec 13 09:04:17 2017 -0600

    test - client-option and fragments complete

commit ada3dd0
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 22:35:42 2017 -0600

    test mutations type check

commit 36cce91
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 22:06:58 2017 -0600

    checkpoint test type checks - queries directory working

commit 953a2dd
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 18:53:37 2017 -0600

    lifecycle.test type checked

commit 725bf50
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 18:13:04 2017 -0600

    checkpoint

commit 3eb018b
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 18:11:11 2017 -0600

    noUnusedParams cannot be used because the simplest react component is (props) => null

commit 2d50050
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 18:03:57 2017 -0600

    errors.test and queries.test work

commit e4f3c79
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 17:47:13 2017 -0600

    api.test back to type checking - seems like other triple slash directives interfered

commit e45ab77
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 17:02:21 2017 -0600

    unify on one tsconfig and just cleanup after compile - reduces chances of differences in test type checking

commit 6c24e71
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 16:51:21 2017 -0600

    run prettier on all changes

commit 070240b
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 16:40:28 2017 -0600

    api.test is working/typechecking

commit a28fa32
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 16:27:27 2017 -0600

    once-over completed on api-test

commit d069d70
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 15:38:54 2017 -0600

    lint-fix

commit 8cbdca6
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 15:37:41 2017 -0600

    formating/lint-staged fighting?

commit 2fa6968
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 15:36:51 2017 -0600

    Expose TChildProps on the outside, allow inference to determine props on the inside

commit 94ba1f3
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 15:21:32 2017 -0600

    graphql inference looks to working (so far so good)

commit 7a54d29
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 14:35:54 2017 -0600

    Fix graphql decorator inference and tie together parameterized types (breaking change)

    Fixes issue apollographql#755, Supercedes PR apollographql#1395 - and making progress towards valid typescript in the test directory apollographql#1404

commit 78b949a
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 14:03:52 2017 -0600

    fix unnecessary redefinition of React.ComponentType as CompositeComponent

commit a37ccfc
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 12:54:44 2017 -0600

    New jasmine custom matcher added - typescript still unhappy about it.

    - Reorganized generic typings file into a folder with module per file layout
    - Moved jest setup files under test/setup (with matcher)

commit a7b4d4a
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Tue Dec 12 03:07:21 2017 -0600

    revert prettier config to default line length (not matching tslint’s 140 max)

commit 47115af
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 20:43:27 2017 -0600

    revert tsconfig lib back to es2015 from es2017

commit abba30b
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 20:30:57 2017 -0600

    Remove unused enzyme-to-json

commit 8f21aed
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 20:13:59 2017 -0600

    broken - Symbol(id) is making it into props.data

commit a098135
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:42:10 2017 -0600

    fix duplicate/conflicting export of getDataFromTree namespace

commit 9cd83a4
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:14:52 2017 -0600

    revert danger to 1.2.0 for the moment

commit 33d10d0
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:11:32 2017 -0600

    babel-preset-react-native updated

commit 7b415ef
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:08:43 2017 -0600

    cheerio updated

commit b02a834
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:07:38 2017 -0600

    lint-staged, danger updated

commit 1867b60
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:02:26 2017 -0600

    redux-form and redux-loop upgraded

commit 7f6affa
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 19:01:04 2017 -0600

    babel-jest and ts-jest upgrade success

commit 72fa15b
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:57:03 2017 -0600

    add changelog to satisfy danger before continuing

commit 497f2f8
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:49:59 2017 -0600

    reuse tsconfig.json for type checking to avoid false positives

    Change lib to 2017 fixes the AsyncIterable problem

commit 8c9f591
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:25:42 2017 -0600

    make sure sources are linted in the `test` script

commit fdedf5d
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:24:43 2017 -0600

    fix lint error

commit bad455f
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:21:04 2017 -0600

    fix cli deprecation for tslint

commit 824c5b2
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:18:44 2017 -0600

    run lint-fix (prettier + tslint fix)

commit 3e3b125
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 18:14:09 2017 -0600

    move prettier to config file for external tool use, add tslint —fix to lint-fix script

commit 4ea6b7a
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 17:52:25 2017 -0600

    Update all non-major versions

commit 561a319
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 17:13:06 2017 -0600

    typos

commit 844356e
Author: Kevin Ross <kevin.ross@alienfast.com>
Date:   Mon Dec 11 16:50:05 2017 -0600

    Fix TS4053 error with latest @types/react

    Error:(45, 3) TS4053: Return type of public method from exported class has or is using name 'React.ReactElement' from external module "react-apollo/node_modules/@types/react/index" but cannot be named.
@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 14, 2017

@orta I updated to 2.1.4 - so perhaps that is the problem - any guide for 2.x and TS? I saw a bunch of TS related docs removed from the repo.

@orta

This comment has been minimized.

Copy link

orta commented Dec 14, 2017

Should "just work" will clone and look now.

@orta

This comment has been minimized.

Copy link

orta commented Dec 14, 2017

Interesting, so the results of running the ts transpiler with your tsconfig.json seems to be that it produces imports instead of requires.

e.g. with the tsconfig, and with an empty config.

screen shot 2017-12-14 at 2 17 21 pm

Digging into see if there's something I'm doing wrong in Danger


I think in this case Danger needs to override/remove the module option in a user's tsconfig. Can't see a use-case where it should be needed.

@orta

This comment has been minimized.

Copy link

orta commented Dec 14, 2017

OK, want to try updating to 2.1.5 please?

When I update & run node --inspect node_modules/danger/distribution/commands/danger-pr.js https://github.com/apollographql/apollo-client/pull/2476 with that in your PR - it passes

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 14, 2017

Thanks @orta! danger ran without a problem.

@orta

This comment has been minimized.

Copy link

orta commented Dec 14, 2017

Thanks for poking me - I had a PR on our RN repo which exhibited the same issue artsy/emission#804

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 14, 2017

@jbaxleyiii all green, ready for review. In the meantime I'm working with the package in our codebase to find any inconsistencies with the types.

@rosskevin rosskevin changed the title WIP Dependencies update, typescript check tests, fix hoc inference Dependencies update, typescript check tests, fix hoc inference Dec 14, 2017

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 14, 2017

Amazing, committed a script change some other test fails. We need a better way than timing, because clearly travis is inconsistent.

@rosskevin

This comment has been minimized.

Copy link
Collaborator Author

rosskevin commented Dec 14, 2017

I'm never committing again for fear of breaking the time-dependent tests on travis 😳

@jbaxleyiii jbaxleyiii self-assigned this Dec 18, 2017

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Dec 18, 2017

@rosskevin this is absolutely INCREDIBLE WORK. Thank you SO SO much for showing the imitative and commitment here!

I'm good to merge this!

@jbaxleyiii jbaxleyiii merged commit b6b5bff into apollographql:master Dec 18, 2017

4 checks passed

CLA Author has signed the Meteor CLA.
Details
Danger ⚠️ Danger found some issues. Don't worry, everything is fixable.
Details
bundlesize ./dist/index.min.js: 6.49KB (5.46KB smaller than master, good job!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rosskevin rosskevin deleted the rosskevin:dependencies-update branch Dec 18, 2017

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