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

Import relayOperationGenericsRule #1

Merged
merged 7 commits into from
Oct 16, 2018
Merged

Import relayOperationGenericsRule #1

merged 7 commits into from
Oct 16, 2018

Conversation

ashfurrow
Copy link
Member

This is a work-in-progress PR to import relayOperationGenericsRule.js into this project, including a transition to TypeScript proper instead of @ts-check. It's a WIP because I ran out of time this morning. Next steps are to finish it, link it from Reaction, and get Reaction consuming the rule from a module. Then we can think about tests, too, and then add this and other eslint-plugin-relay rules to this project. And then publish to NPM. Baby steps.

See also: artsy/yeoman-generator-artsy#10

/cc @alloy

@ashfurrow ashfurrow self-assigned this Oct 13, 2018
@ashfurrow-peril
Copy link

ashfurrow-peril bot commented Oct 13, 2018

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by 🚫 dangerJS

@peril-staging
Copy link

peril-staging bot commented Oct 13, 2018

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by 🚫 dangerJS

@ashfurrow
Copy link
Member Author

Aww jeez

package.json Outdated
@@ -48,5 +49,8 @@
"yarn prettier --write",
"git add"
]
},
"dependencies": {
"graphql": "^14.0.2"
Copy link
Member

Choose a reason for hiding this comment

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

It’s customary to make these types of dependencies peer ones, although I think that’s more important when it’s a dep that will be used at runtime to ensure the app has only 1 version of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, interesting! It took until now for me to get the distinction, this blog post was helpful.

Copy link
Member

Choose a reason for hiding this comment

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

It’s an art!

* @param {number} width
* @param {string} replacement
* @param {string} operationName
* @returns {Lint.Replacement[]}
Copy link
Member

Choose a reason for hiding this comment

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

I believe these type annotations can be removed when using TS, which already has these in its function signature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I've been working my way down the file and switching them over 👍

@@ -0,0 +1,198 @@
import { BREAK, parse, visit } from "graphql"
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth it to import this * as graphql as these symbols could be slightly confusing when used together with the tsc API

@alloy
Copy link
Member

alloy commented Oct 14, 2018

Ah yes! Nice 👌

@alloy
Copy link
Member

alloy commented Oct 14, 2018

Btw, I’d appreciate it if you could make me the author of the initial commit (of only the rule file), even if just for knowing who to talk to if there’s ever any questions 🙏

@alloy
Copy link
Member

alloy commented Oct 14, 2018

Let‘s move it to the relay-tools org before cutting the first release. I’ll invite you to the org now.

@ashfurrow
Copy link
Member Author

Sure, sounds good 👍 Thanks!

@alloy
Copy link
Member

alloy commented Oct 15, 2018

Thanks!

"typescript": "^3.0.3"
"tslint-config-prettier": "^1.15.0",
"typescript": "^3.0.3",
"graphql": "^14.0.2"
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to put this in devDependencies because peer deps aren't installed by default.

@ashfurrow ashfurrow changed the title WIP: Import relayOperationGenericsRule Import relayOperationGenericsRule Oct 15, 2018
@ashfurrow
Copy link
Member Author

Okay, this just needs testing now. Before we publish, we can link locally on reaction and make sure it will work. I'm expecting to have to faff with TSLint to get it to load rules from node modules.

@ashfurrow
Copy link
Member Author

I've found this blog post helpful for getting the package set up and linked from Reaction: https://palantir.github.io/tslint/2016/03/31/sharable-configurations-rules.html

Also, it's not possible to link against a package that has no published version (see this issue) so I added the following to Reaction's package.json to test:

"tslint-plugin-relay": "link:../tslint-plugin-relay"

Which sets up the symlink on yarn install.

@ashfurrow
Copy link
Member Author

Okay, the blog post I linked to is a bit outdated since it doesn't permit TypeScript rules. I've modified how we compile to point to the rules directory, for consistency.

It's not working when linked locally, but I think it might be a problem with the module resolution that our symlink is not adhering too. I'm going to merge, move the repo to the relay-tools org, then publish a v0.0.1 so we can test on Reaction.

@ashfurrow ashfurrow merged commit 6be7398 into master Oct 16, 2018
@ashfurrow ashfurrow deleted the initial-work branch October 16, 2018 19:21
@ashfurrow
Copy link
Member Author

Woo! https://www.npmjs.com/package/tslint-plugin-relay

Okay, linking works. I looked and there were two problems:

  • tslint-base.json gets copied to build on yarn build, but references the ./build directory from there, causing path problems.
  • Reaction's tslint.json file needs to extend "tslint-plugin-relay", not "tslint-plugin-relay/tslint-config" as described in the blog post above.

I'll fix both, verify this is working, then PR to Reaction 👍

@ashfurrow ashfurrow mentioned this pull request Oct 16, 2018
ashfurrow added a commit that referenced this pull request Oct 16, 2018
@ashfurrow
Copy link
Member Author

Oh shoot. It looks like a rebase erased the git author status for Eloy on the initial import. I did a force-push on master to correct before anyone sees 🙈

@alloy
Copy link
Member

alloy commented Oct 16, 2018

Woohoo! Nice job, @ashfurrow 👏

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