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

Migrate Query.test to tsx and parameterize Query.tsx #1462

Merged
merged 14 commits into from Dec 26, 2017

Conversation

Projects
None yet
3 participants
@rosskevin
Copy link
Collaborator

rosskevin commented Dec 26, 2017

  • Query.test was js - migrated to tsx and fixed errors by parameterizing Query.tsx with TData
  • Removed custom jest matcher prototyped in #1457 by @excitement-engineer - switched to more explicit/flexible/working utility method
@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Dec 26, 2017

Warnings
⚠️

❗️ Big PR

Generated by 🚫 dangerJS

@excitement-engineer
Copy link
Collaborator

excitement-engineer left a comment

Awesome work @rosskevin! Thanks a lot for fixing this. I have left some small comments but it looks pretty good so far.

@@ -10,7 +10,7 @@ cache:
before_install:
- npm install -g npm@^3 --cache-min 999999999
- npm install -g coveralls --cache-min 999999999
- time npm i -g yarn --cache-min 999999999

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Collaborator

Why did you remove this in the PR?

This comment has been minimized.

@rosskevin

rosskevin Dec 26, 2017

Author Collaborator

We don't use yarn at all. Why install it alongside npm?

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Collaborator

Good point! I think that the travis.yml could be cleaned up a bit as well, lot of unused stuff in there.

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Collaborator

good for a follow-up PR, this is on my todo list.

This comment has been minimized.

@rosskevin

rosskevin Dec 26, 2017

Author Collaborator

BTW - I use yarn, I just didn't want to unnecessarily change here. But I don't mind switching if others want to.

@@ -0,0 +1,9 @@
const catchAsyncError = (done, cb) => {

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Collaborator

There is also a wrap in the test-utils that does something very similar. Could we merge these into one single method? Perhaps an idea for a follow-up PR?

This comment has been minimized.

@rosskevin

rosskevin Dec 26, 2017

Author Collaborator

We could merge - I just extracted because it wasn't necessary here.

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Collaborator

Great. Maybe an idea to do it in a follow-up PR to keep the code clean and prevent duplication of methods from appearing.

"lint-staged": "lint-staged",
"lint-fix":
"npm run pretty && tslint 'src/*.ts*' --project tsconfig.json --fix",
"pretty":

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 26, 2017

Collaborator

Just a small tip. But could you try and keep your PR as condensed as possible so that you are fixing a single thing in a PR. So I would prefer to see unrelated changes like this in a separate PR. I prefer to keep the PRs as small as possible in order to maximize the quality of the review.

@rosskevin rosskevin merged commit e78f9a0 into apollographql:master Dec 26, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./lib/react-apollo.browser.umd.js: 6.39KB (11B smaller than master, good job!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rosskevin rosskevin deleted the rosskevin:query-typescript branch Dec 26, 2017

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