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

refactor: Rewrite in TypeScript. #39

Merged
merged 9 commits into from
Dec 28, 2018
Merged

refactor: Rewrite in TypeScript. #39

merged 9 commits into from
Dec 28, 2018

Conversation

avocadowastaken
Copy link
Contributor

No description provided.

@avocadowastaken
Copy link
Contributor Author

avocadowastaken commented Dec 21, 2018

Published with #21 to https://npmjs.com/@dc0de/react-apollo-hooks

@trojanowski
Copy link
Owner

trojanowski commented Dec 22, 2018

Thanks @umidbekkarimov for your great work here. I'll do a full review after Christmas, but right now I have 2 suggestions:

  1. I'd like to have the git history and changelog (generated by standard-version) easy to follow so I'd like to merge it using the "Squash and merge" option. It would be great if this PR just renames the current files and adds TS types there (so index.js -> index.ts etc.), and splitting hooks to separate files is done in a follow-up PR.
  2. I'd use TSLint instead of ESLint for linting the TypeScript code. It seems more popular for that case and it's also already used in Apollo repositories. I'm using this configuration in my projects (maybe it needs some customization):
{
  "defaultSeverity": "error",
  "extends": ["tslint:latest", "tslint-react", "tslint-config-prettier"],
  "jsRules": {},
  "rules": {
    "interface-name": false,
    "interface-over-type-literal": false,
    "jsx-boolean-value": [true, "never"],
    "jsx-no-lambda": false,
    "max-classes-per-file": false,
    "member-access": false,
    "no-implicit-dependencies": true,
    "no-submodule-imports": false,
    "no-this-assignment": [true, { "allow-destructuring": true }],
    "no-unused-expression": [true, "allow-fast-null-checks"],
    "only-arrow-functions": false,
    "ordered-imports": [
      true,
      {
        "import-sources-order": "lowercase-last",
        "named-imports-order": "lowercase-last"
      }
    ],
    "prefer-template": true
  },
  "rulesDirectory": []
}

Please let me know if you don't have time for that - I'd do it myself in the next week in that case.

@avocadowastaken
Copy link
Contributor Author

avocadowastaken commented Dec 22, 2018

I'd like to have the git history and changelog (generated by standard-version) easy to follow so I'd like to merge it using the "Squash and merge" option. It would be great if this PR just renames the current files and adds TS types there (so index.js -> index.ts etc.), and splitting hooks to separate files is done in a follow-up PR.

Done, I tried save git history as much as possible in force pushed commits but git still wants to "remove and add" index.tsx and objToKey.ts files.

I'd use TSLint instead of ESLint for linting the TypeScript code. It seems more popular for that case and it's also already used in Apollo repositories. I'm using this configuration in my projects (maybe it needs some customization):

Done.

I'll do a full review after Christmas

Happy Holidays!

Copy link
Owner

@trojanowski trojanowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have the git history and changelog (generated by standard-version) easy to follow so I'd like to merge it using the "Squash and merge" option. It would be great if this PR just renames the current files and adds TS types there (so index.js -> index.ts etc.), and splitting hooks to separate files is done in a follow-up PR.

Done, I tried save git history as much as possible in force pushed commits but git still wants to "remove and add" index.tsx and objToKey.ts files.

Thank you. Anyway, it was easier to compare.

I added some comments. I'm going to split index.ts to separate files after merging this PR (similar how is it done in #42 - I'm going to review that one tomorrow).

tsconfig.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.tsx Outdated
useState,
} from 'react';
import isEqual from 'react-fast-compare';
import { Omit, assertApolloClient } from './Utils';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always try to group builtin node modules (not used here), external dependencies and internal ones together, separated by a blank line. I haven't found a tslint rule or config for that but I think is a good practice. I'd also change that in queryCache.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is the solution https://github.com/renke/import-sort? I has similar api as prettier: --write and --list-different.

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Show resolved Hide resolved
src/__testutils__/noop.ts Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
@avocadowastaken
Copy link
Contributor Author

@trojanowski Thanks, It's good to have you back! I've resolved your comments.

I hate to bring it up, code style changes (e.g. sorting of imports) should be done by tools, not humans. Prettier spoiled me.

@trojanowski
Copy link
Owner

Thank you @umidbekkarimov

I added one comment about the new changes.

I hate to bring it up, code style changes (e.g. sorting of imports) should be done by tools, not humans. Prettier spoiled me.

I agree. Thanks for the tip about import-sort - I'll try to add it.

@avocadowastaken
Copy link
Contributor Author

I added one comment about the new changes.

@trojanowski I can't find any, maybe you forgot to submit it?

src/index.tsx Outdated Show resolved Hide resolved
@trojanowski
Copy link
Owner

@trojanowski I can't find any, maybe you forgot to submit it?

Yes, sorry. It should be already visible.

@avocadowastaken
Copy link
Contributor Author

@trojanowski Good catch, done. Also 3 another PRs are on the way 🙂

src/index.tsx Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@trojanowski
Copy link
Owner

I added one last-minute comment. I think it's the last one :)

@avocadowastaken
Copy link
Contributor Author

No problem, it's never easy to review 20+ file PRs. Fixed.

@avocadowastaken
Copy link
Contributor Author

Forgot to push 🤦‍♂️

@trojanowski trojanowski merged commit 055f0e2 into trojanowski:master Dec 28, 2018
@trojanowski trojanowski requested review from trojanowski and removed request for trojanowski December 28, 2018 20:47
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