wip: Discussions #15

nicksnyder · 2018-09-16T01:11:39Z

This is not mergeable for a few reasons:

uses proposed api that is not finalized (we will need to publish our extension as preview or create a separate preview extension for dogfooding, https://sourcegraph.slack.com/archives/C0LJP8WTB/p1537195769000100)
There is some hard-coded access tokens for local development
I am not sure of all the semantics of the revisions we pass to Sourcegraph. Needs vetting by @slimsag
Does not handle local modifications to files (should limit commenting range to non-modified ranges?)
Add logic to refresh known threads periodically

slimsag · 2018-09-16T01:13:20Z

uses proposed api that is not finalized (I think we can't publish with this, unless maybe we mark this extension as preview? Unclear.)

My hope is that we can get this working behind e.g. a feature flag and at least ship to everyone on our team. I'll try to figure out if this is possible or not

slimsag · 2018-09-16T01:16:08Z

Does not handle local modifications to files (should limit commenting range to non-modified ranges?)

I'll think about this some more.

For reading existing comments, it should be easy for us to support placing the comments in the right locations on modified files (for now, we can do this by sending the code in your editor to the sourcegraph server. In the future, before we ship to any real users, this would be done client side using the same line resolution logic).

For creating new comments, the biggest risk is probably just making sure that we clearly denote "I am commenting on some modified code" in the DB so we don't end up with a lot of garbage that we later need to filter out somehow on web. I'll think of how to do this.

nicksnyder · 2018-09-16T01:17:02Z

The GitHub Pull Requests extension got published with proposed api, so unsure if they got a special exception or if we just need to mark our extension as preview

src/git.ts

slimsag · 2018-09-16T01:23:41Z

src/graphql.ts

@@ -0,0 +1,70 @@
+'use strict'


/rant GraphQL is supposed to be nicer than REST and gRPC but then you end up having to copy code like this all around the place and end up having to write your own functions to do fetches like replyToCommentThread anyway.

You dont have to copy paste this code. We could share it. Some things in here are also unneeded, like the symbol-backed gql tag.
We could generate functions for queries and mutations, but if you need to specify what fields you want

slimsag · 2018-09-16T01:29:17Z

I will push a commit to unify some of the GraphQL logic here to share it a bit more with https://github.com/sourcegraph/sourcegraph-extension-discussions

slimsag · 2018-09-16T05:01:41Z

sourcegraph/sourcegraph#13253 will address the whole repo resolution issue, will push a change here soon to implement it on this end

cp ../sourcegraph-extension-discussions/src/typings/SourcegraphGQL.d.ts src/typings/SourcegraphGQL.d.ts

nicksnyder · 2018-09-17T10:25:25Z

I am not sure I agree with the "shared" code introduced in 246b109.

For one thing, the extension is now broken (will investigate).
The shared code also includes weird "deprecated" but still used stuff.
More generally, it seems appropriate for clients to define exactly what fields they need instead of a superset of everything.

I would prefer to revert that change unless you actually want to split it into a shared dependency (which I would still have some concerns with). The proverb "a little bit a copying is better than a little bit of dependency" comes to mind.

felixfbecker · 2018-09-17T10:31:32Z

I agree, the reason GraphQL clients are usually not generated is that the GraphQL query language itself is seen as the "interface" or "SDK" for APIs. A folder that must be kept in sync sounds like it causes more trouble than it offers benefits.

slimsag · 2018-09-17T17:11:04Z

For one thing, the extension is now broken (will investigate).

It's not broken for me. What is broken for you?

The shared code also includes weird "deprecated" but still used stuff.

I agree, that sucks. It is code copied from our main repo. However, before this change I was receiving arbitrary errors such as:

Invalid GraphQL response for DiscussionThreads

With no information about what errors the GraphQL endpoint actually returned. And debugging this wasn't trivial for me because the error handling implementation was 100% different than the other two implementations.

More generally, it seems appropriate for clients to define exactly what fields they need instead of a superset of everything.

The vast majority of GraphQL fields included here are:

Already in use.
Not in use, but extremely cheap to compute and transfer.
Not in use, but should be.

There are now three separate TypeScript clients (web app, Sourcegraph Extension, VS Code extension) that:

Share and require 95% of the same GraphQL fields.
Share the same requirements of:
- "Give me all the threads"
- "Add a comment to a thread, give me the updated version"
- "Create a new thread and give it to me"

In what amounts to a few hundred lines of non-trivial code that can be messed up easily in each client IMO.

From my perspective, I have trouble seeing this as anything other than the most perfect use case for sharing code.. but maybe I just don't understand "the GraphQL way" and copying code here is better. I am happy to concede here.

felixfbecker · 2018-09-17T17:16:12Z

cc @dadlerj who worked on GraphQL error handling

dadlerj · 2018-09-17T18:17:49Z

src/shared/api.ts

+        { input, relativeRev }
+    )
+    if (!data || !data.discussions || !data.discussions.createThread) {
+        throw createAggregateError(errors)


We have nicer error handling now using dataOrThrowErrors! See https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/src/backend/graphql.tsx#L21:17 and examples in https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/commit/78e0791ca3e3d4ac37d13b38c6da84473cb6c4e5

dadlerj · 2018-09-17T18:17:56Z

src/shared/api.ts

+        opts
+    )
+    if (!data || !data.discussionThreads) {
+        throw createAggregateError(errors)


dadlerj · 2018-09-17T18:18:02Z

src/shared/api.ts

+        !data.discussionThreads.nodes ||
+        data.discussionThreads.nodes.length !== 1
+    ) {
+        throw createAggregateError(errors)


dadlerj · 2018-09-17T18:18:06Z

src/shared/api.ts

+        { threadID, contents, relativeRev }
+    )
+    if (!data || !data.discussions || !data.discussions.addCommentToThread) {
+        throw createAggregateError(errors)


dadlerj · 2018-09-17T18:18:10Z

src/shared/api.ts

+        { markdown }
+    )
+    if (!data || !data.renderMarkdown) {
+        throw createAggregateError(errors)


slimsag · 2018-09-17T18:56:17Z

I've thought about this a lot more over the past few hours. Here is my final conclusion / stance on the topic:

I am actually OK with duplicating the GraphQL queries / fragments / function definitions and having three separate copies. I'm not happy about it, and I think it is ugly / dumb that GraphQL inherently forces us to do this, but I am OK with this because there isn't a better generalizable approach.

But, I strongly dislike the fact that we have separate error handling and GraphQL request code that is copied, sprinkled, and modified throughout several of our codebases, and Dan's feedback above just re-enforces my stance here that it is impossible to know which file to copy.

To be specific, why does the browser extension still use createAggregateError? How do we know the version of graphql.ts copied into this repo by Nick is the correct version? Why must all of these implementations be different and each contain their own bugs or imperfections in their own various ways?:

sourcegraph-vscode: Nick's graphql.ts file here that was obviously copied from some other implementation (who knows where?).
sourcegraph-extension-discussions: The graphql.ts file I wrote for the Sourcegraph Extension, which sucks / doesn't support the gql marker syntax because implementing these files is hard.
sourcegraph/sourcegraph: The graphql.tsx file we have in the webapp repo which apparently has better error handling today than createAggregateError according to Dan above.
extensions-client-common: The graphql.ts file we have in the extensions-client-common repo, which is apparently using its own beautiful fork of createAggregateError.
browser-extensions: Yet another graphql.tsx and errors.tsx fork. Is it better than the webapp's implementation? Why doesn't it use extensions-client-common's version?

I think this is a problem worth solving before we merge this PR for the following reasons:

I encountered issues with the error handling here as I stated earlier.
With this PR, we introduce yet another copy, and with sourcegraph-extension-discussions we introduce yet another copy. The problem is rapidly growing and getting out of hand.
Even if we don't update all of the codebases, it seems like this problem would be easy to solve by publishing just one implementation here as an npm package (if we knew which one was the best one!).

felixfbecker · 2018-09-17T19:23:38Z

I agree that it's not good that these are copied everywhere. We have a lot of copy-pasted code, I've been working on sharing more, and GraphQL copied query functions were always minor compared to things like hover tooltip code. They are literally tiny HTTP request functions. It's easy to copy, not as easy to come up with the right abstraction. For example, how do you even write a query function, that works both in webapp/browser extension, but also in NodeJS, without relying on platform-specific API like fetch or pulling in big dependencies, while also maintaining features like cancellation? How does the perfect GraphQL error handling even look like? There is nothing inherently wrong with createAggregateError(). When writing a query function, it should just make sure that if it cannot fulfil its contract, it throws an error with enough context, like every function. If a query function can't return the data it's supposed to return because the backend returned an error, it should throw with that error data. If the webapp achieves this goal in a more advanced way than our small vscode extension, that is not a big problem per se.

felixfbecker · 2018-09-17T19:36:13Z

There are also open source solutions out there that we could and probably should use. I will just need some time to evaluate them.

slimsag · 2018-09-17T19:44:39Z

The short answer is: I don't know. I don't have the answers here. I am just calling out what is from my perspective a major pain point in using GraphQL in our codebase, that I feel should be easily addressable somehow.

I tried to address this by sharing the code between this extension and sourcegraph-extension-discussions but it is hard because in the case of Sourcegraph Extensions the GraphQL requests have to be done through a VS Code-like command API not through fetch, so I didn't know how to publish this as an npm package easily. And my approach obviously wasn't generalizable to the other codebases.
Writing sourcegraph-extension-discussions, I didn't know how to write these queryGraphQL, mutateGraphQL functions and I couldn't simply copy the others. Figuring out how to write these properly would've taken me several hours (and writing the entire extension itself only took me 5, so that is not acceptable).
Working on this PR the error handling just didn't work, and the codebase was completely different than other codebases I've worked on with GraphQL error handling. So I guess I should've debugged what was wrong in this PR instead of trying to share the code(?) but I feel like that would have again taken me a few hours to figure out and that time was better spent elsewhere.

nicksnyder · 2018-09-17T20:10:00Z

I am getting this graphql error: "invalid graphql.ID" when creating a new thread.

mutation CreateThread($input: DiscussionThreadCreateInput!, $relativeRev: String!) {
          discussions {
            createThread(input: $input) {
              ...DiscussionThreadFields
              comments {
                totalCount
                nodes {
                  ...DiscussionCommentFields
                }
              }
            }
          }
        }
        
    fragment DiscussionThreadFields on DiscussionThread {
      id
      author {
        ...UserFields
      }
      title
      target {
        __typename
        ... on DiscussionThreadTargetRepo {
          repository {
            name
          }
          path
          branch {
            displayName
          }
          revision {
            displayName
          }
          selection {
            startLine
            startCharacter
            endLine
            endCharacter
            linesBefore
            lines
            linesAfter
          }
          
        relativePath(rev: $relativeRev)
        relativeSelection(rev: $relativeRev) {
          startLine
          startCharacter
          endLine
          endCharacter
        }
    
        }
      }
      inlineURL
      createdAt
      updatedAt
      archivedAt
    }

    fragment UserFields on User {
      displayName
      username
      avatarURL
    }
  
        
    fragment DiscussionCommentFields on DiscussionComment {
      id
      author {
        ...UserFields
      }
      contents
      html
      inlineURL
      createdAt
      updatedAt
    }

{input: {title: "asd", contents: "asd", targetRepo: {repositoryGitCloneURL: "git@github.com:nicksnyder/go-i18n", path: "v2/i18n/bundle.go", branch: "master", …}}, relativeRev: "master"}

nicksnyder · 2018-09-17T20:14:51Z

sourcegraph-vscode: Nick's graphql.ts file here that was obviously copied from some other implementation (who knows where?).

The bulk of this file is requestGraphQL, which is not copied because I had to make it work in a node environment. The convenience functions and gql tag were copied, but there is not much there.

And debugging this wasn't trivial for me because the error handling implementation was 100% different than the other two implementations.

I spent no time thinking about or dealing with edge/error cases (this was hack quality). I suspect it will be hard to share error code because the right way to treat errors in a VS Code extension may be different than web.

slimsag · 2018-09-17T20:15:00Z

@nicksnyder Odd, that must mean that the repositoryGitCloneURL bit is failing for some reason but I have no clue why that would be or why it would be producing such a lame error. Are you running the latest sourcegraph/sourcegraph master branch with my change for repositoryGitCloneURL merged in?

slimsag · 2018-09-17T20:16:11Z

@nicksnyder that's totally fair :) I just feel we gotta get error handling working properly before merging this

nicksnyder · 2018-09-17T20:17:18Z

I am running sourcegraph/enterprise, do I need to run sourcegraph/sourcegraph?

slimsag · 2018-09-17T20:18:12Z

no, you should run $ENTERPRISE/dev/start.sh but make sure you have an up to date copy of sourcegraph/sourcegraph cloned in your gopath

nicksnyder · 2018-09-17T20:28:41Z

make sure you have an up to date copy of sourcegraph/sourcegraph cloned in your gopath

Oops, this was my problem 😬

slimsag · 2018-10-04T07:05:23Z

Moving to #19

nicksnyder added 3 commits September 15, 2018 18:26

refactor: wip

5c5ca51

refactor: working

bffb82d

refactor: working?

591f359

nicksnyder requested a review from slimsag September 16, 2018 01:11

refactor: cleanup

4d600e1

slimsag reviewed Sep 16, 2018

View reviewed changes

src/git.ts Outdated Show resolved Hide resolved

refactor: prettier?

ae8217d

slimsag reviewed Sep 16, 2018

View reviewed changes

nicksnyder mentioned this pull request Sep 16, 2018

refactor: git and config logic to separate file #16

Merged

nicksnyder and others added 9 commits September 15, 2018 21:36

Merge branch 'master' into discussions

fa1eecb

refactor: remove debug info

0da5171

refactor: remove launch hack

97aaabb

refactor: remove rxjs

63da797

refactor: remove 'use strict'

a0fc9c0

refactor: clarify repoInfo function behavior + improve logging

cae4364

Merge branch 'master' into discussions

bfe9910

Merge branch 'master' into discussions

e0ecee7

rm semicolon

77ef314

slimsag added 3 commits September 16, 2018 00:13

refactor: update graphql

22dd51e

cp ../sourcegraph-extension-discussions/src/typings/SourcegraphGQL.d.ts src/typings/SourcegraphGQL.d.ts

refactor: share graphql code with Sourcegraph Extension

246b109

chore: npm run prettier

296f887

dadlerj reviewed Sep 17, 2018

View reviewed changes

nicksnyder mentioned this pull request Oct 4, 2018

Code discussions #19

Closed

slimsag closed this Oct 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: Discussions #15

wip: Discussions #15

nicksnyder commented Sep 16, 2018 •

edited

slimsag commented Sep 16, 2018

slimsag commented Sep 16, 2018

nicksnyder commented Sep 16, 2018

slimsag Sep 16, 2018

felixfbecker Sep 16, 2018

slimsag commented Sep 16, 2018

slimsag commented Sep 16, 2018

nicksnyder commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

slimsag commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

dadlerj Sep 17, 2018

dadlerj Sep 17, 2018

dadlerj Sep 17, 2018

dadlerj Sep 17, 2018

dadlerj Sep 17, 2018

slimsag commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

slimsag commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

slimsag commented Sep 17, 2018

slimsag commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

slimsag commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

slimsag commented Oct 4, 2018

wip: Discussions #15

wip: Discussions #15

Conversation

nicksnyder commented Sep 16, 2018 • edited

slimsag commented Sep 16, 2018

slimsag commented Sep 16, 2018

nicksnyder commented Sep 16, 2018

slimsag Sep 16, 2018

Choose a reason for hiding this comment

felixfbecker Sep 16, 2018

Choose a reason for hiding this comment

slimsag commented Sep 16, 2018

slimsag commented Sep 16, 2018

nicksnyder commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

slimsag commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

dadlerj Sep 17, 2018

Choose a reason for hiding this comment

dadlerj Sep 17, 2018

Choose a reason for hiding this comment

dadlerj Sep 17, 2018

Choose a reason for hiding this comment

dadlerj Sep 17, 2018

Choose a reason for hiding this comment

dadlerj Sep 17, 2018

Choose a reason for hiding this comment

slimsag commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

felixfbecker commented Sep 17, 2018

slimsag commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

slimsag commented Sep 17, 2018

slimsag commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

slimsag commented Sep 17, 2018

nicksnyder commented Sep 17, 2018

slimsag commented Oct 4, 2018

nicksnyder commented Sep 16, 2018 •

edited